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

Allow custom-size-icons #9263

Closed
wants to merge 3 commits into from
Closed

Allow custom-size-icons #9263

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
426f0a6
Allow custom-size-icons
jasmussen Aug 23, 2018
7084075
chore: Tweak comments
tofumatt Aug 23, 2018
cb17db8
chore: tweak comments
tofumatt Aug 23, 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
11 changes: 10 additions & 1 deletion packages/editor/src/components/block-icon/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,17 @@
}
}

svg:not(.dashicon) {
// By default, library icons should be 24px by 24px.
// If an icon is provided that has a `dashicons` class, or an explicit width/height,
// we don't override the height/width.
svg:not(.dashicon):not([width]) {
Copy link
Member

Choose a reason for hiding this comment

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

Based on the comments, shouldn't this be:

svg:not(.dashicon),
svg:not([height]),
svg:not([width]) {

As it is an explicit height alone would still be overridden, and the docs imply that an explicit width OR height will prevent it–even if the dashicon class is there. That's how I read it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the 24px width/height needs to be applied to the container, otherwise the height of the whole item—when using a smaller icon—is wrong in the inserter:

edit_page_ mindful _wordpress

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As it is an explicit height alone would still be overridden, and the docs imply that an explicit width OR height will prevent it–even if the dashicon class is there. That's how I read it.

Your point that this comment needs clarification is solid. I got a headache just trying to walk through this now. I might need your help to clarify my code :D

So, this is what we want to accomplish:

  • If you just dump in an SVG and don't really think about it, it's 24x24 as it should be
  • If you dump in a dashicon, we know that's 20x20 so we don't enforce 24x24
  • If you specify an explicit width (or height, do we have to check for that, seems like we could assume with a width?) — then sure, go ahead and have a custom sized icon so long as it's smalller than 24x24.

Hmm. Actually, your code should do the same shouldn't it?

How would you write this as a comment? Thanks so much for your help!

height: 24px;
width: 24px;
}

// Don't allow icons to be bigger than the recommended standard.
svg {
max-width: 24px;
max-height: 24px;
}
}
11 changes: 10 additions & 1 deletion packages/editor/src/components/inserter-with-shortcuts/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,19 @@
.components-icon-button {
border-radius: $radius-round-rectangle;

svg:not(.dashicon) {
// By default, library icons should be 24px by 24px.
// If an icon is provided that has a `dashicons` class, or an explicit width/height,
// we don't override the height/width.
svg:not(.dashicon):not([width]) {
height: 24px;
width: 24px;
}

// Don't allow icons to be bigger than the recommended standard.
svg {
max-width: 24px;
max-height: 24px;
}
}
}

Expand Down