Add language chooser package and component #64686
Conversation
|
Size Change: +4.15 kB (+0.23%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
swissspidy
added
the
No Core Sync Required
|
@aaronjorbin @youknowriad What do you think about this? |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Moves all callbacks to the higher level. Moves all the tests too.
swissspidy
changed the title
Can be handled in core
packages/language-chooser/src/language-chooser/active-controls.tsx
Outdated
Show resolved
Hide resolved
packages/language-chooser/src/language-chooser/active-controls.tsx
Outdated
Show resolved
Hide resolved
packages/language-chooser/src/language-chooser/active-locales.tsx
Outdated
Show resolved
Hide resolved
|
|
||
| // WordPress grays. | ||
| $black: #000; // Use only when you truly need pure black. For UI, use $gray-900. | ||
| $gray-900: #1e1e1e; |
There was a problem hiding this comment.
Could we use the pre-existing --wp-components-color-gray- theme colors somehow?
$gray-900 is also deprecated there in favor of foreground and we have background that uses white.
There was a problem hiding this comment.
You mean --wp-components-color-foreground?
I can't use a white background.
This component is supposed to live in wp-admin where the background is #F0F0F1.
| align-items: center; | ||
| } | ||
|
|
||
| .language-chooser__active-locales-list li { |
There was a problem hiding this comment.
I'm a bit concerned if we're using consistent sizing, spacing and colors. Since I'm not a specialist, I think it would make sense to get a review from @WordPress/gutenberg-design here.
There was a problem hiding this comment.
Thanks for the patience @swissspidy!
I spent some time testing it, and it works great! Nice work 🙌
Code also looks good 👍
I only have a minor comment on the announcements used when moving or removing active languages, but it's not a blocker.
I think if anything comes up, we can always address it as a follow-up.
Did we need a final ✅ from design?
🚀
| ); | ||
| } ); | ||
|
|
||
| speak( __( 'Locale moved down' ) ); |
There was a problem hiding this comment.
Does it make sense for these "locale moved down/up" and "locale removed" strings to contain the language we're moving up or down? In my testing with assistive technology, it's not immediately clear which locale I'm moving if I forgot which one was announced as the "selected" one previously.
|
Personally, I don't think this should be in a dedicated component. It should be in the components package, we shouldn't be creating components for single UI components. |
|
We had this discussion already 😅 |
I kindly disagree - we don't want to make the components package a catch-all kitchen sink - new high-level and app components like this shouldn't belong together with the generic, general-purpose components in the components package. |
I think the maintenance cost of 1 component = 1 package is too big. If you don't consider this one a component that is a generic UI component, we can also consider it a component that is an editor specific component and move it to where it's going to be used. Where is that? block-editor? editor? |
I agree that 1 component = 1 package adds unneeded maintenance cost if we do it for many more components in the future, and that this is something to be mindful about. Ideally, new high-level or app components should live closer to the app code, just like block-editor, editor and other app-related packages. See my prior thoughts on that. I was OK to accept a separate package for this particular one, exactly because it doesn't belong in the editor / block-editor, or any other existing package. Also see @jsnajdr's earlier thoughts which I was fine with as a compromise to move this work forward, and we all agreed earlier in the PR to move forward with. |
That is actually a great option for me. Why was this option discarded? |
Seems like @swissspidy didn't prefer it. I was initially OK with that option, and I see some others were in favor of it. However, the relation is thin between |
|
Ok, so my preference is still that this is a generic UI component personally a While I'm ok with a npm package for a component, I think a Anyway, if my arguments don't convince you, happy to disagree and commit. |
That's a good point, maybe we should also discuss the global variable under which the package is exposed at runtime and what policy should govern them. We have things like |
I agree that this could be abstracted to such a component, but personally, I'd consider doing that only if we observe a real need for it in more than one place. There would also be more implications if we added a new component to
My suggestion is to pick a place for this higher-level component to exist (it's been discussed above and I don't personally have a strong opinion). And when/if there is need to abstract it to a generic component, we can create such a component in |
💯 I see no further reason to delay this work. I'd suggest moving forward, and if, at some point, there's a need for the underlying UI component to be abstracted, we can consider doing it following the plan @ciampo outlined above. |
Except that it would be too late and we'll be forced to support a non ideal API/package in WordPress. Let's move forward though, I feel like I made my point. |
| isRemoveDisabled, | ||
| }: ActiveControlsProps ) { | ||
| return ( | ||
| <ButtonGroup> |
There was a problem hiding this comment.
We're actually in the process of deprecating ButtonGroup (context), so it would be better to replace it with another component (maybe a VStack or a simple vanilla HTML element).
There was a problem hiding this comment.
Of course it is. I just switched to it after it was recommended to use it. Guess I‘ll switch to something else now.
There was a problem hiding this comment.
@swissspidy feel free to blame me for this one.
I recommended using ButtonGroup as a better alternative to an unordered list, but I did it before we decided that ButtonGroup would be deprecated.
|
Mmm, now I'm more confused about the rationale to not have it in components. 😀 I don't think @ciampo point about a more thorough design review should be specific to where components are placed. Is the idea that we'd be more lax on a standalone component? If the question is about not having a need to reuse it, that's applicable to it being a package in the first place. There seems to be some fundamental disagreements on what the packages stand for that we should try—at the risk of bike-shedding—to get on the same page before pushing public APIs. My own impression remains that this is a fine wp/component to have provided it's documented and annotated well. Alternatively, with the addition of wp/fields, is language picking something that fits there more coherently? |
I agree that a design review should be carried out regardless. I left specifically this feedback in my first reply, where I tagged the design group and questioned whether this is the UI/UX that we should land on for a "sortable, multiple selection" list, but no-one followed up.
Agreed that we need to fund consensus around the scope of each package.
We already shared many comments in this PR (including some from me, like this one and this one), but the TL;DR of my personal point of view is:
|
What?
Context: https://make.wordpress.org/core/2024/05/07/merge-proposal-preferred-languages/
Adds a new
LanguageChoosercomponent for use in WordPress core (site settings, user profile, network settings, potentially even the install screen).Update: after a lot of back and forth, this is now a new
@wordpress/language-chooserpackage.Core then only needs the "glue" code rendering the component and PHP implementation to make it work. I have a local POC for that already that I will be trying to push to the WordPress develop GitHub repo soon.
Then, these two PRs can be tested together to verify everything works correctly.
Why?
In the discussion it was determined that the Gutenberg repository is the best home for this new UI component as it already has proper build tooling in place to support TypeScript, Jest, etc. These are not available in core. And Gutenberg is the de facto JS monorepo for core anyway.
How?
Adds a new
LanguageChoosercomponent to@wordpress/components.Core can then use that component & its styles to display the Preferred Languages language chooser on the settings page.
Testing Instructions
Typical interactions:
Testing Instructions for Keyboard
All buttons, listbox, dropdowns, etc. can be interacted with using keyboard only. There are dedicated shortcuts screen reader announcements as well.
Screenshots or screencast
In storybook:
Integrated into WordPress admin: