Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Polish permalink UI and make it responsive #9010

Merged
merged 8 commits into from
Aug 16, 2018
Merged

Polish permalink UI and make it responsive #9010

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
481026c
Fix input and focus style for permalink slug
Aug 15, 2018
63a23a7
Fix regression with horizontal scrollbar on mobile.
Aug 15, 2018
d069ac5
Use serious flexing to scale the permalink editor.
Aug 15, 2018
bab5c54
chore: Fix typo
tofumatt Aug 15, 2018
ab3c628
chore: Wording tweak
tofumatt Aug 15, 2018
f5c3696
Clarify comments.
Aug 15, 2018
f0e3b9d
Address feedback.
Aug 16, 2018
4c35997
chore: Fix failing CSS Lint test
tofumatt Aug 16, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions edit-post/assets/stylesheets/main.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,7 @@ body.gutenberg-editor-page {
// Override core input styles to provide styles consistent with Gutenberg.
// Upstream ticket for redesigned input and styles in general: https://core.trac.wordpress.org/ticket/44749
// Upstream ticket for redesigned checkbox: https://core.trac.wordpress.org/ticket/35596
.editor-post-permalink,
.edit-post-sidebar,
.editor-post-publish-panel,
.editor-block-list__block,
Expand Down
29 changes: 20 additions & 9 deletions packages/editor/src/components/block-list/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,9 +198,9 @@
transition: outline 0.1s linear;
pointer-events: none;

// Show wider padding for top level blocks.
right: -$parent-block-padding;
left: -$parent-block-padding;
// Go edge-to-edge on mobile.
right: -$block-padding;
left: -$block-padding;
top: -$block-padding;
bottom: -$block-padding;

Expand Down Expand Up @@ -780,11 +780,18 @@
> .editor-block-list__insertion-point {
position: absolute;
top: -$block-padding - $block-spacing / 2;
// Matches the whole empty space between two blocks

// Matches the whole empty space between two blocks.
height: $block-padding * 2;
bottom: auto;
left: -$border-width;
right: -$border-width;
left: 0;
right: 0;

// Stack with block borders beyond mobile.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not quite sure what this comment means, and it references code aimed at a mobile viewport but talks about what happens outside the mobile viewport. I get that it means mobile behaves one way and others differently, but I think this comment could be clarified 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Solid point, will rearrange and clarify that a bit.

THANKs for all the fixes and review!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed your comment in f5c3696

@include break-small() {
left: -$border-width;
right: -$border-width;
}
}

&[data-align="full"] > .editor-block-list__insertion-point {
Expand Down Expand Up @@ -854,8 +861,8 @@
}

// Make block toolbar full width on mobile.
margin-left: -$border-width;
margin-right: -$border-width;
margin-left: 0;
margin-right: 0;

@include break-small() {
margin-left: -$parent-block-padding - $parent-block-padding - $border-width;
Expand Down Expand Up @@ -962,9 +969,13 @@
z-index: z-index(".editor-block-list__breadcrumb");

// Position in the top right of the border.
right: -$block-parent-side-ui-clearance;
right: 0;
top: -$block-padding - $border-width;

@include break-small() {
right: -$block-parent-side-ui-clearance;
}

// Nested
.editor-block-list__block-edit & {
right: $parent-block-padding - $block-padding - $block-parent-side-ui-clearance;
Expand Down
2 changes: 1 addition & 1 deletion packages/editor/src/components/post-permalink/editor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ class PostPermalinkEditor extends Component {
className="editor-post-permalink-editor"
onSubmit={ this.onSavePermalink }
>
<span>
<span className="editor-post-permalink-editor__container">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's close, but this is not quite following the naming standards. Specifically, the part preceding the __ isn't concerned at all with the file name, only the top-level folder and the immediate parent folder. It should be editor-post-permalink__editor-container.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking again at those standards, we need to update them to be less... editor-specific.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed feedback for now, perhaps we can revisit the editor naming separately?

<span className="editor-post-permalink-editor__prefix">
{ prefix }
</span>
Expand Down
33 changes: 29 additions & 4 deletions packages/editor/src/components/post-permalink/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,9 +55,29 @@
display: inline-flex;
align-items: center;

.editor-post-permalink-editor__container {
flex: 0 1 100%;
display: flex;
overflow: hidden; // This enables serious flex shrinking.
padding: $border-width 0; // Necessary for the overflow to not crop the focus style.

.editor-post-permalink-editor__prefix {
flex: 1 1 auto;

@include break-small {
flex: 1 0 auto;
}
}

.editor-post-permalink-editor__edit {
flex: 1 1 100%;
}
}

// Higher specificity required to override core margin styles.
.editor-post-permalink-editor__save {
margin-left: auto;
flex: 0 1 auto;
}
}

Expand All @@ -70,12 +90,17 @@
text-overflow: ellipsis;
}

.editor-post-permalink-editor__edit {
min-width: 20%;
margin: 0 5px;
.editor-post-permalink input[type="text"].editor-post-permalink-editor__edit {
// Input fields are created with inherent widths.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was the cause of the failing test; fixing it now.

// By supplying both a (any) width and a min-width, we allow it to scale in a flex container.
min-width: 10%;
width: 100%;
margin: 0 3px;
padding: 2px 4px;
}

.editor-post-permalink-editor__suffix {
color: $dark-gray-300;
margin-right: 10px;
margin-right: 6px;
flex: 0 0 0%;
}