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

URL input popover visual cleanup #13973

Merged
merged 6 commits into from
Feb 21, 2019
Merged

URL input popover visual cleanup #13973

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
47acdb2
Use chevron instead of ellipsis in url input field options.
kjellr Feb 19, 2019
a35b7a7
Mimic toolbar icon styles for the icons in the URL popover.
kjellr Feb 19, 2019
6cdf20b
Add a left divider before the URL settings toggle
kjellr Feb 19, 2019
3fb56cd
Even up the spacing in the settings panel.
kjellr Feb 20, 2019
b12a17f
Add periods to code comments.
kjellr Feb 21, 2019
02acf7e
Update snapshot
kjellr Feb 21, 2019
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
2 changes: 1 addition & 1 deletion packages/editor/src/components/url-popover/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ class URLPopover extends Component {
{ !! renderSettings && (
<IconButton
className="editor-url-popover__settings-toggle"
icon="ellipsis"
icon="arrow-down-alt2"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just noting, I think our panels actually use a material SVG here, but the IconButton doesn't support that yet, so I'm using the closest equivalent from Dashicons.

label={ __( 'Link Settings' ) }
onClick={ this.toggleSettingsVisibility }
aria-expanded={ isSettingsExpanded }
Expand Down
46 changes: 40 additions & 6 deletions packages/editor/src/components/url-popover/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,19 +9,53 @@
flex-grow: 1;
}

// Mimic toolbar component styles for the icons in this popover.
.components-icon-button {
padding: 3px;

> svg {
padding: 5px;
border-radius: $radius-round-rectangle;
height: 30px;
width: 30px;
}

&:not(:disabled):not([aria-disabled="true"]):not(.is-default):hover {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another note: This is a rather unfortunate string of :not conditions, but it's needed to override the existing styles on the standard IconButton. See the component styles here for reference:

gutenberg/packages/components/src/icon-button/style.scss

Lines 28 to 30 in 14374e5

&:not(:disabled):not([aria-disabled="true"]):not(.is-default):hover {
@include button-style__hover;
}

box-shadow: none;

> svg {
@include formatting-button-style__hover;
}
}

&:not(:disabled):focus {
box-shadow: none;

> svg {
@include formatting-button-style__focus;
}
}
}

&__settings-toggle {
Copy link
Contributor

Choose a reason for hiding this comment

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

I know you didn't write this initially, but while you're here do you think we should change this rule to be less obtuse? Although there isn't any written rule saying this much, in general I believe the SCSS standard pattern that is emerging is to use write .editor-url-popover__settings-toggle rather than .editor-url-popover { &__settings-toggle { ... } }.

Hmm actually that might be a big rewrite of this CSS file. Okay to ignore if you feel this is too big of a task.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed! This was really confusing to parse. I'll take care of this in a separate PR, so we can get this change in first. 👍

flex-shrink: 0;
width: $icon-button-size;
height: $icon-button-size;

.dashicon {
transform: rotate(90deg);
// Add a left divider to the toggle button.
border-radius: 0;
border-left: $border-width solid $light-gray-500;
margin-left: 1px;

&[aria-expanded="true"] .dashicon {
transform: rotate(180deg);
}
}

&__settings {
padding: 7px 8px;
padding: $panel-padding;
Copy link
Contributor

Choose a reason for hiding this comment

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

💖

border-top: $border-width solid $light-gray-500;
padding-top: 7px + $border-width;

.components-base-control:last-child .components-base-control__field {
margin-bottom: 0;
}
}
}
4 changes: 2 additions & 2 deletions packages/editor/src/components/url-popover/test/__snapshots__/index.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ exports[`URLPopover matches the snapshot in its default state 1`] = `
<IconButton
aria-expanded={false}
className="editor-url-popover__settings-toggle"
icon="ellipsis"
icon="arrow-down-alt2"
label="Link Settings"
onClick={[Function]}
/>
Expand All @@ -40,7 +40,7 @@ exports[`URLPopover matches the snapshot when the settings are toggled open 1`]
<IconButton
aria-expanded={true}
className="editor-url-popover__settings-toggle"
icon="ellipsis"
icon="arrow-down-alt2"
label="Link Settings"
onClick={[Function]}
/>
Expand Down