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.

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

Replace metabox arrows with Dashicon chevrons for greater consistency. #16100

Closed
wants to merge 3 commits into from

Conversation

kjellr
Copy link
Contributor

@kjellr kjellr commented Jun 11, 2019

This is a half-step towards closing #15881.


The expand/collapse arrows for custom meta boxes are currently not in sync with the Material chevron icons used in the rest of Gutenberg:

Screen Shot 2019-06-11 at 3 56 34 PM

This is because the arrows are Dashicons, brought in via core's common.css. Since we don't have an iconfont version of the usual expand/collapse arrows we use in Gutenberg, this PR pulls in the closest Dashicon alternatives:

Screen Shot 2019-06-11 at 3 53 16 PM

The Dashicon chevrons are almost the same as the ones we use. Considering how similar they are (and how small the icons are here), I think it makes sense to switch to these ones until we can implement a more permanent fix.

Untitled-1

@kjellr kjellr added Needs Design Feedback Needs general design feedback. [Feature] Meta Boxes A draggable box shown on the post editing screen labels Jun 11, 2019
@kjellr kjellr requested a review from jasmussen June 11, 2019 23:06
@kjellr kjellr self-assigned this Jun 11, 2019
@kjellr kjellr requested a review from mapk June 11, 2019 23:15
@jasmussen
Copy link
Contributor

I do kind of wonder if a half step makes sense. I kind of think no-one but you and me will notice that the dashicon isn't the same as the material chevron. At the same time, it does bug me :D

But I also don't want to block this PR as it's a good step towards consistency, so on that note I wouldn't object. But I still wonder, what would it take to make the full step?

@kjellr
Copy link
Contributor Author

kjellr commented Jun 12, 2019

But I still wonder, what would it take to make the full step?

I'm not totally sure. It appeared to me that those controls are basically just inherited from WP-Admin. I think someone would have to rebuild with pure Gutenberg components, which doesn't seem like a huge priority for us at the moment? I wonder if @youknowriad or @noisysocks has some idea (I see they've worked on the meta-boxes-area component before).

Also, @jasmussen, another option would be to swap out the existing chevrons with Material ones in Dashicons itself... we're already using an incongruent one for the back button in fullscreen mode, and I can't think of a great argument for why they should be slightly different.

@jasmussen
Copy link
Contributor

Also, @jasmussen, another option would be to swap out the existing chevrons with Material ones in Dashicons itself... we're already using an incongruent one for the back button in fullscreen mode, and I can't think of a great argument for why they should be slightly different.

There's a larger discussion there, largely summarized in this comment, which suggests mostly that path for WordPress as a whole, Material icons plus dashicons to fill in the gaps. But Dashicons as they are today are incompatible because of the smaller (20x20) pixel grid than Material (24x24).

The real solution is of course to refactor out the use of Dashicons in that particular metabox instance, but I would agree that's a larger undertaking.

In any case, no real objections, just good to muse about this! :)

@noisysocks
Copy link
Member

noisysocks commented Jul 4, 2019

In theory, fixing this for reals should just be a matter of changing the <span> here to be a <svg>:

https://github.com/WordPress/wordpress-develop/blob/5fea17ba03c42d16a6517b164d3437bd53ac8ac9/src/wp-admin/includes/template.php#L1277

So, something like:

if ( $current_screen->is_block_editor() ) {
	echo '<svg class="toggle-indicator" aria-hidden="true"><path>...</path></svg>';
} else {
	echo '<span class="toggle-indicator" aria-hidden="true"></span>';
}

(Or maybe you don't need an if if CSS could swap out the svg's content in the classic editor? Not sure.)

This would have to be done in Trac and not here in the Gutenberg repo.

@chrisvanpatten
Copy link
Contributor

I think a good clarifying question here is asking how the project leads see the future of meta boxes?

My interpretation was that meta box support in Gutenberg was provided primarily as a way to ease a transition to more Gutenberg-y implementations (e.g. sidebar API, blocks, etc). If that is still the case, they are effectively a deprecated feature and I’m not sure it makes sense to make accommodations and affordances for them. The slight design variations are a reminder that they are not native to Gutenberg, and they can (and will) behave differently from other elements of the interface.

@noisysocks
Copy link
Member

cc. @youknowriad

@mapk
Copy link
Contributor

mapk commented Jul 24, 2019

If that is still the case, they are effectively a deprecated feature and I’m not sure it makes sense to make accommodations and affordances for them. The slight design variations are a reminder that they are not native to Gutenberg, and they can (and will) behave differently from other elements of the interface.

This is a fair discussion and good points being raised. I'd agree that they will be deprecated at some future point. But this time is probably a ways off still. For this reason, I would love to see them more aligned to the Gutenberg interface + icons. The quick change to Dashicons chevrons works for me here as providing a bit of consistency in the interim. I'd avoid going to Trac with this in Core.

@chrisvanpatten
Copy link
Contributor

I guess I'll just voice my objection again — WordPress has a long history of pulling code into the future far beyond its usefulness. With the recent addition of a SlotFill which now allows truly Gutenberg-native panels in the Document Settings panel, there is finally a full end-to-end migration path for custom metaboxes. With this new addition, there is very little justification for someone to use legacy metabox code, and I think making a design affordance for the legacy approach is a mistake.

@kjellr
Copy link
Contributor Author

kjellr commented Jul 24, 2019

I guess I'll just voice my objection again — WordPress has a long history of pulling code into the future far beyond its usefulness. With the recent addition of a SlotFill which now allows truly Gutenberg-native panels in the Document Settings panel, there is finally a full end-to-end migration path for custom metaboxes. With this new addition, there is very little justification for someone to use legacy metabox code, and I think making a design affordance for the legacy approach is a mistake.

I definitely appreciate the sentiment of the objection here, and we definitely want folks to natively implement their custom meta boxes.

But from a user's perspective, they generally wouldn't know or care if these boxes are using a legacy approach or not. They look similar enough to the rest of the editor that the inconsistency in the expand/collapse arrows reads as just a design oversight. It's all just a single interface, in the eyes of the user.

@paaljoachim
Copy link
Contributor

A few comments as I see it.
Do please switch out the metaboxes arrows with the ones used other places such as the sidebar.

Metaboxes are very likely still used by a lot of users in probably many different ways. We still need to continue the support for a few (likely a bunch) years and perhaps find a way to create a better transition of the older code used in the metaboxes to a call it version 2 metaboxes.
This is a potential discussion for another time.

Go go.... get the new arrows in place for the metaboxes..:)

@mapk mapk added Needs Dev Ready for, and needs developer efforts and removed Needs Design Feedback Needs general design feedback. labels Mar 6, 2020
@draganescu
Copy link
Contributor

I also approve of this, and while I understand @chrisvanpatten 's perspective it does look like an unfinished job to the user. @kjellr did anything new happen in the meantime (new UI, icons etc) that should prevent us from merging this?

@github-actions
Copy link

Size Change: +118 B (0%)

Total Size: 842 kB

Filename Size Change
build/edit-post/style-rtl.css 12.4 kB +59 B (0%)
build/edit-post/style.css 12.4 kB +59 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.02 kB 0 B
build/annotations/index.js 3.62 kB 0 B
build/api-fetch/index.js 4.08 kB 0 B
build/autop/index.js 2.82 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 6.24 kB 0 B
build/block-directory/style-rtl.css 760 B 0 B
build/block-directory/style.css 761 B 0 B
build/block-editor/index.js 105 kB 0 B
build/block-editor/style-rtl.css 10.2 kB 0 B
build/block-editor/style.css 10.2 kB 0 B
build/block-library/editor-rtl.css 7.1 kB 0 B
build/block-library/editor.css 7.1 kB 0 B
build/block-library/index.js 112 kB 0 B
build/block-library/style-rtl.css 7.17 kB 0 B
build/block-library/style.css 7.18 kB 0 B
build/block-library/theme-rtl.css 683 B 0 B
build/block-library/theme.css 685 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 57.7 kB 0 B
build/components/index.js 198 kB 0 B
build/components/style-rtl.css 16.9 kB 0 B
build/components/style.css 16.9 kB 0 B
build/compose/index.js 6.66 kB 0 B
build/core-data/index.js 11.4 kB 0 B
build/data-controls/index.js 1.25 kB 0 B
build/data/index.js 8.43 kB 0 B
build/date/index.js 5.47 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 569 B 0 B
build/dom/index.js 3.1 kB 0 B
build/edit-navigation/index.js 3.54 kB 0 B
build/edit-navigation/style-rtl.css 485 B 0 B
build/edit-navigation/style.css 485 B 0 B
build/edit-post/index.js 27.9 kB 0 B
build/edit-site/index.js 10.5 kB 0 B
build/edit-site/style-rtl.css 5.04 kB 0 B
build/edit-site/style.css 5.04 kB 0 B
build/edit-widgets/index.js 7.5 kB 0 B
build/edit-widgets/style-rtl.css 4.67 kB 0 B
build/edit-widgets/style.css 4.66 kB 0 B
build/editor/editor-styles-rtl.css 428 B 0 B
build/editor/editor-styles.css 431 B 0 B
build/editor/index.js 43.3 kB 0 B
build/editor/style-rtl.css 3.28 kB 0 B
build/editor/style.css 3.27 kB 0 B
build/element/index.js 4.65 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.32 kB 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/hooks/index.js 2.13 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.56 kB 0 B
build/is-shallow-equal/index.js 711 B 0 B
build/keyboard-shortcuts/index.js 2.51 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.12 kB 0 B
build/list-reusable-blocks/style-rtl.css 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/media-utils/index.js 5.29 kB 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.4 kB 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/plugins/index.js 2.67 kB 0 B
build/primitives/index.js 1.49 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/redux-routine/index.js 2.84 kB 0 B
build/rich-text/index.js 14.8 kB 0 B
build/server-side-render/index.js 2.67 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.28 kB 0 B
build/url/index.js 4.02 kB 0 B
build/viewport/index.js 1.84 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

@kjellr
Copy link
Contributor Author

kjellr commented Apr 22, 2020

did anything new happen in the meantime (new UI, icons etc) that should prevent us from merging this?

Yeah, the chevron icon has changed, so the dashicon one is less of a match. The difference is more obvious, but it's still better than what's there today. I'd get some feedback from @mapk or @jasmussen on whether this is an improvement or not.

Current:

Screen Shot 2020-04-22 at 8 58 15 AM

This PR:

Screen Shot 2020-04-22 at 8 54 44 AM

@jasmussen
Copy link
Contributor

Thanks for keeping on this. This has been out of mind for me for a while. Since then with some of the G2 efforts, I've been wanting to look at panels, and try to reduce borders and icons whenever possible. I don't have a clear picture of how the sidebar could be refined, but the short is that I would expect it to change further with some of the G2 designs Pablo created in #18667. So we may want to keep this one sitting for a while further :|

Alternately — what would it take to fix the problem at the root? Actually use the Icon component, or just bake in the SVGs, in the metabox panels themselves?

Base automatically changed from master to trunk March 1, 2021 15:42
@annezazu
Copy link
Contributor

Closing this out due to lack of activity and the age of this effort. Feel free to reopen or create a new PR if folks want to continue this work.

@annezazu annezazu closed this Jul 27, 2022
@youknowriad youknowriad deleted the update/metabox-chevrons branch September 7, 2022 09:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Meta Boxes A draggable box shown on the post editing screen Needs Dev Ready for, and needs developer efforts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants