Removed hardcoded sizes on icon svgs #41772

danieliser · 2022-06-16T22:37:01Z

What?

There were a handful of icon svgs that had hard coded sizes set to 24.

Why?

These hardcoded sizes prevented Icon size & Button iconSize props from working as expected.

How?

Simply removed those sizes.

ntsekouras

Thanks for the PR @danieliser!

The changes look good to me and I have left a small comment for some designer's input.

ntsekouras · 2022-06-20T10:42:22Z

packages/icons/src/library/line-dashed.js

@@ -4,7 +4,7 @@
 import { Path, SVG } from '@wordpress/primitives';

 const lineDashed = (
-	<SVG xmlns="http://www.w3.org/2000/svg" width="24" height="24" fill="none">
+	<SVG xmlns="http://www.w3.org/2000/svg" fill="none">


Should we add viewBox="0 0 24 24" to the icons that don't have it? --cc @jameskoster

Yes I believe so 👍

@danieliser could you please add the viewbox to the updated icons? It's not related to your PR, but it's a small thing to add here 😄

Sure thing, will check the others as well.

@ntsekouras Do they all need a fill="none" as well, or is that conditionally based on the icons design?

Also found the format-ltr & format-rtl have non standard sizes of viewBox="-2 -2 24 24", hesitant to change those without some confirmation

Same for insertAfter, insertBefore,keyboardClose,keyboardReturn,plusCircle,tableColumnAfter, tableColumnBefore,tableColumnDelete, tableRowAfter, tableRowBefore, tableRowDelete, update, warning, & finally wordpress.

Sorry thought there were less when I started, might as well be comprehensive once I got halfway through lol.

Along the lines of standardizing, I went ahead and sorted props the same way for each with xmlns followed by viewBox and finally fill

For reference, standardizing order of props makes it easier to use Find & Replace to look for discrepancies.

Thanks for the changes! I don't think changing the order was needed in this PR, but there is no harm in it. Initially I meant add it only in the updated icons.

You should rebase with trunk because there are conflicts from icons that have been removed now and it should be good to land.

packages/icons/src/library/line-dashed.js

packages/icons/src/library/line-dotted.js

packages/icons/src/library/line-solid.js

* Block Editor: Abstract selection position retrieval * Add tests for retrieveSelectedAttribute * Refactor away from _.findKey * Deprecate _.findKey() * Remove autogenerated docs * Make experimental * Revert "Make experimental" This reverts commit 3fe8e9d. * Keep internal

…ustom post type and specific posts templates (#41189) * WIP: [Site Editor]: Expand the template types that can be added * Fix typo * Remove icons in general vs specific selection modal * Update post lookup styling to match linkcontrol * Fix modal width, set suggestion list height * fix linting * remove `archive` additions and check for available posts by excluding the existing ones before adding the menu item * add safeguard for `existingTemplates` * add link in suggestions list * clean up comments * update formatting after trunk change * use gutenberg_get_block_template for updates in templates * add `icon` in Post Types REST API and handle only dashicons in the templates list for now * use `rest_prepare_post_type` filter * address feedback part 1 * update jsdoc * remove mixin * try different labels * Apply suggestions from code review Co-authored-by: Miguel Fonseca <miguelcsf@gmail.com> * address feedback Co-authored-by: James Koster <james@jameskoster.co.uk> Co-authored-by: Miguel Fonseca <miguelcsf@gmail.com>

…ML (#41516) * #30930 - Make it easier to select "Edit visually" when in "Edit as HTML" mode * BlockVisuallyConvertButton name converted to BlockEditVisuallyButton * title removed from the <ToolbarButton/> component * code formatting fix * Reversed buttons order * divider between the button and the ellipsis * Update packages/block-editor/src/components/block-settings-menu/block-edit-visually-button.js Co-authored-by: Adam Zielinski <adam@adamziel.com> Co-authored-by: Zebulan Stanphill <zebulanstanphill@protonmail.com>

* ColorIndicator: Convert component to TypeScript * Update CHANGELOG.md * Apply suggestions from code review Co-authored-by: Lena Morita <lena@jaguchi.com> * Clearify colorValue in readme file * Remove unused exclude from story * Add forwardref * Update CHANGELOG.md * Remove export * Update CHANGELOG.md Co-authored-by: Lena Morita <lena@jaguchi.com>

…trigger (#41774) * Add escape key functionality for navigation submenus. * Fix to selector. * Add missing toggle focus to submenu close function. * Code comment.

…41820) * Autocomplete: revert exhaustive deps refactor * Autocomplete: update changelog * Autocomplete: temporarily disable exhaustive-deps * remove testing artifact

* Initial commit Adding border style types and generator functions Refactoring generateBoxRules to accommodate various "individual" properties * Added explainer comment for BorderIndividualStyles type Removing lodash camelCase and upperFirst in favour of own implementation + test Removing unnecessary lodash get()

* Add Pocket Casts embed block variation * Add embed block variation for Pocket Casts * Add the Pocket Casts icon on embed icons file * Update Pocket Casts embed block variation * Update the block description * Add audio keywork to the block * Change the Pocket Casts icon to use the brand color * Remove unnecessary pocketcasts keyword

* Format Library: Add keyboard shortcut for inline code * Update KeyboardShortcuts help modal * Update snapshots

* Add ui for button elements * prettier fixes * attempt to bring background color for button styling * set background color for button element * enables multiple color stack in the color indicator * use zstack for multiple color display, cleanup the UI for multiple color selection * Elements: Fix button element selectors * add missing files * lint fix * add comment Co-authored-by: Andrei Draganescu <andrei.draganescu@automattic.com>

* Block attributes docs: consistency across attribute definitions * Block attributes docs: use tabs * Block attributes docs: unify code blocks descriptions * Block attributes docs: add 'source' * Block attributes docs: nest 'Meta source' heading one level deeper It's the source too, and other sources are placed on the `h3` level. The nested heading is also updated.

* Add ui for button elements * enables multiple color stack in the color indicator * use zstack for multiple color display, cleanup the UI for multiple color selection * remove useless styling for section title * remove duplicate comment Co-authored-by: Andrei Draganescu <andrei.draganescu@automattic.com>

danieliser · 2022-07-11T00:06:16Z

wow that was not supposed to get published, gonna have to delete this branch and redo it off trunk again, not sure I can undo that rebase. Misclicked when publish in VSC when I was looking for delete 🤦‍♂️

danieliser · 2022-07-11T00:14:00Z

Ok so the rebase does appear to have gone correctly, but during the conflict resolution I mistakenly added 3 icon files that seem to have been deleted from trunk. Those are the only changes in this branch that are not correctly in line.

If someone can fill me in on best strategy to remove those without cherry picking all the rest, would appreciate that.

Do I just remove them in a follow up commit? They are query-title, archive-title & post-title icon files.

noisysocks · 2022-07-11T01:22:02Z

Ok so the rebase does appear to have gone correctly, but during the conflict resolution I mistakenly added 3 icon files that seem to have been deleted from trunk. Those are the only changes in this branch that are not correctly in line.

Did you push after re-rebasing? The current state of the PR doesn't look correct as there are more than 250 commits and over 10,000 changed lines in it.

If someone can fill me in on best strategy to remove those without cherry picking all the rest, would appreciate that.

Do I just remove them in a follow up commit? They are query-title, archive-title & post-title icon files.

Yes you can make these changes as additional commits and then push those to the PR.

danieliser · 2022-07-11T01:28:23Z

I think I pushed mid rebase somehow. It said no more changes in VS Code, but I had to manually clear the rebase.

So I'm guessing that means that the rebase was actually just a dirty merge of all those commits as part of this PR 🤦‍♂️

danieliser · 2022-07-11T01:29:34Z

I'm finding my understanding of rebase seems to be off. I mean I know what its supposed to be used for, but not understanding exactly what its doing under the hood leaves me questioning whether it worked or not.

draganescu · 2022-07-11T07:22:27Z

1005 files changed 😁 definitely did not work. My suggestion (which I apply myself) is to separate your original changes, reset this branch to trunk, reapply your changes in one commit and push the branch. This way the intent and meaning of the PR is restored.

VS Code

I found GUI clients handle rebase pretty flakey too. But since the PR when merged has the commits squashed anyway you can apply the way I described above and fix this PR 👍🏻

t-hamano · 2022-08-31T15:12:55Z

Hi @danieliser,

This PR appears to improve on the three line-(dashed|dotted|solid) icons.
But these icons seem to have been resolved by #40315 and merged into trunk.

Therefore, I am very sorry, but I would like to close this PR.
If there is anything I have missed about the changes you have made, please do not mind commenting.
Thanks for your contribution!

Mamaduka added the [Package] Icons /packages/icons label Jun 17, 2022

ntsekouras reviewed Jun 20, 2022

View reviewed changes

ntsekouras reviewed Jun 21, 2022

View reviewed changes

packages/icons/src/library/line-dashed.js Outdated Show resolved Hide resolved

ntsekouras reviewed Jun 21, 2022

View reviewed changes

packages/icons/src/library/line-dotted.js Outdated Show resolved Hide resolved

ntsekouras reviewed Jun 21, 2022

View reviewed changes

packages/icons/src/library/line-solid.js Outdated Show resolved Hide resolved

tyxla and others added 25 commits June 21, 2022 11:19

Try to fix the image rotating flaky test (#41808)

6250a1f

Lodash: Refactor away from _.findLast() (#41809)

78247df

Lodash: Refactor away from _.nth() (#41832)

df8e783

Navigation submenus: Allow Escape key to close the submenu and focus …

db8b006

…trigger (#41774) * Add escape key functionality for navigation submenus. * Fix to selector. * Add missing toggle focus to submenu close function. * Code comment.

Revert: refactor Autocomplete component to pass exhaustive-deps (#…

eb62ba5

…41820) * Autocomplete: revert exhaustive deps refactor * Autocomplete: update changelog * Autocomplete: temporarily disable exhaustive-deps * remove testing artifact

FormToggle: Covert component to TypeScript (#41729)

7f8ff80

Upgrade Aztec to v1.5.9 (#41828)

d86eaa4

Lodash: Refactor away from _.uniqWith() (#41812)

0f97e8d

Format Library: Add keyboard shortcut for inline code (#41816)

ca3f229

* Format Library: Add keyboard shortcut for inline code * Update KeyboardShortcuts help modal * Update snapshots

Lodash: Remove completely from annotations package (#41856)

a243966

Lodash: Remove completely from wordcount package (#41863)

c3213b1

Lodash: Remove completely from token-list package (#41864)

e542907

Fix some small inconsistencies in the block editor docs (#41861)

2a7c820

Editors: Add a link completer shortcut to the help modals (#41872)

e460b95

Rendering duotone presets in pattern preview (#41249)

4315382

Bump plugin version to 13.5.0

2a40d2d

danieliser requested review from adamziel, kevin940726, alexstine, dmsnell, atimmer, michalczaplinski, mmtr, ajlende, juanmaguitar and fabiankaegy as code owners July 11, 2022 00:04

michalczaplinski removed request for adamziel, juanmaguitar, atimmer, mmtr, ajlende, michalczaplinski, dmsnell, kevin940726, tellthemachines, alexstine and fabiankaegy August 11, 2022 21:29

Mamaduka mentioned this pull request Aug 31, 2022

Button: iconSize prop doesn't work with some icons #43745

Closed

t-hamano closed this Aug 31, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removed hardcoded sizes on icon svgs #41772

Removed hardcoded sizes on icon svgs #41772

danieliser commented Jun 16, 2022

ntsekouras left a comment

ntsekouras Jun 20, 2022

jameskoster Jun 20, 2022

ntsekouras Jun 20, 2022

danieliser Jun 20, 2022

danieliser Jun 20, 2022 •

edited

Loading

danieliser Jun 20, 2022

danieliser Jun 20, 2022

danieliser Jun 20, 2022 •

edited

Loading

ntsekouras Jun 21, 2022

danieliser commented Jul 11, 2022

danieliser commented Jul 11, 2022

noisysocks commented Jul 11, 2022

danieliser commented Jul 11, 2022

danieliser commented Jul 11, 2022

draganescu commented Jul 11, 2022

t-hamano commented Aug 31, 2022

Removed hardcoded sizes on icon svgs #41772

Removed hardcoded sizes on icon svgs #41772

Conversation

danieliser commented Jun 16, 2022

What?

Why?

How?

ntsekouras left a comment

Choose a reason for hiding this comment

ntsekouras Jun 20, 2022

Choose a reason for hiding this comment

jameskoster Jun 20, 2022

Choose a reason for hiding this comment

ntsekouras Jun 20, 2022

Choose a reason for hiding this comment

danieliser Jun 20, 2022

Choose a reason for hiding this comment

danieliser Jun 20, 2022 • edited Loading

Choose a reason for hiding this comment

danieliser Jun 20, 2022

Choose a reason for hiding this comment

danieliser Jun 20, 2022

Choose a reason for hiding this comment

danieliser Jun 20, 2022 • edited Loading

Choose a reason for hiding this comment

ntsekouras Jun 21, 2022

Choose a reason for hiding this comment

danieliser commented Jul 11, 2022

danieliser commented Jul 11, 2022

noisysocks commented Jul 11, 2022

danieliser commented Jul 11, 2022

danieliser commented Jul 11, 2022

draganescu commented Jul 11, 2022

t-hamano commented Aug 31, 2022

danieliser Jun 20, 2022 •

edited

Loading

danieliser Jun 20, 2022 •

edited

Loading