Add a new InlineLabelSelect component for WCPay admin filter select inputs (with inline label)
#8896
Conversation
Test the buildOption 1. Jetpack Beta
Option 2. Jurassic Ninja - available for logged-in A12s🚀 Launch a JN site with this branch 🚀 ℹ️ Install this Tampermonkey script to get more options. Build info:
Note: the build is updated when a new commit is pushed to this PR. |
|
Size Change: 0 B Total Size: 1.25 MB ℹ️ View Unchanged
|
| * https://github.com/WordPress/gutenberg/tree/7aa042605ff42bb437e650c39132c0aa8eb4ef95/packages/components/src/custom-select-control | ||
| * | ||
| * It has been forked from the existing WooPayments copy of this component (client/components/custom-select-control) | ||
| * to match this specific select input design with an inline label and option hints. |
There was a problem hiding this comment.
Would be good to add to this comment a rough plan for how we might merge this upstream or reduce number of forks in WooPayments code base 😁
There was a problem hiding this comment.
Update: I've paused my review because I think I need to review the diff from the Gutenberg code (and maybe other WooPayments CustomSelectControl forks. @Jinksi how do you think we should handle this?
| */ | ||
| import './style.scss'; | ||
|
|
||
| export interface Item { |
There was a problem hiding this comment.
Item is pretty amorphous, can we give this a clearer name? I like DropdownItem, but that's because I'm familiar with macOS UI patterns/naming.
Looks like the current naming for this component in mac is Pop-up button. So if we follow that the items would be PopupButtonItems. Or if we stick with Select, SelectItems, or SelectControlItems.
There was a problem hiding this comment.
The name item was used by the Gutenberg CustomSelectControl. I assume this naming was used to match the downshift library API and naming.
However, there is no harm in changing the name of the TS Item interface to be more descriptive. I will make this change (00940a3).
| style?: React.CSSProperties; | ||
| } | ||
|
|
||
| export interface Props< ItemType > { |
There was a problem hiding this comment.
Curious about the decision to templateise Props.
- Do we want to allow different
ItemTypes? - What does that allow us to do?
- What does the TypeScript look like if we lock it down to a single
SelectComponentProps, is that a better fit for our use case/user model?
I just realised that I'm probably commenting on the Gutenberg code, likely not worthwhile revisiting the Gutenberg code.
There was a problem hiding this comment.
This was carried over from the WooPayments CustomSelectControl. Custom item types and components are a requirement for onboarding country selection. I'm leaving this feature to allow onboarding screens to more easily adopt this new component.
This has been an unusual challenge of this implementation – I've intentionally kept most of the naming, API, logic and styling from either Gutenberg or the existing WooPayments I'll see if I can find a suitable way to differentiate the changes from Gutenberg → WooPayments → this PR. |
Firstly, the diff between the Gutenberg (JSX) → WooPayments
Secondly, the diff between the WooPayments cc @haszari. I'll add this information to the PR description to help illustrate what has changed. |
|
Awesome, thank you @Jinksi – those diffs are really instructive. I'm going to review and leave some raw notes on those diffs. Then switch focus to reviewing this PR from our project point of view.
Summary of the changes:
Excluding the custom item type, I'm more confident that there's a path to using the WP component direct – most differences are minimal, and not really woo or payments specific. The custom component … need to understand this better, and explore if there are objections/risks to that flexibility in Gutenberg. Best case maybe that feature is useful in core! Possible next step:
Side note: our use of TypeScript might be a barrier to merging upstream. We should find out what WP core TypeScript policy is and if they are JS only, add a carve-out in our guidelines for code borrowed from core. (I will add a comment about this to TypeScript discussion internally) |
Summary of changes:
The check mark option seems doable to add as an option. CSS would need to be explored, might be challenges there. |
It was almost a css-only change, which I initially attempted, but the inline label and show/hiding behaviour of the selected value were the main differences that required markup changes.
Since the design of these component variants is complete we can discuss them with Gutenberg before a PR is opened. Also, with @Automattic/moltres working on the next iteration of this component, we may want to see what implementation we settle on. |
Not sure what you mean by "before" – a PR is the a great way to start a discussion. But not a priority for us for this project, unless it makes it easier to get our component shipshape!
Understandable. Now that you've implemented this behaviour (these behaviours?), how would you expose them as an API or prop? i.e feature(s) that a client of
|
I mean, we can show Gutenberg the new design and ask if it's something that could replace or sit alongside the existing But I agree, a PR is great since it shows the code and APIs as well. This would be best to achieve once we settle on a shared API across our use-cases (including onboarding). |
That's one option; we could also advocate for our needs independently – e.g. divide and conquer strategy 🚀 |
Screen.Recording.2024-06-06.at.13.55.41.movThis is just CSS (although the inline label requires some markup changes), but since WooPayments doesn't currently require the alternative, I just made the changes. But we may want to make this a choice/prop for a Gutenberg contribution. |
Thanks for explaining – that's really clear now 👍🏻 . I can see that this might be considered custom behaviour that others might not need. @Jinksi Please add this info clearly to the PR description and title. That new custom behaviour is IMO a big reason for this PR (i.e. looking back, this PR will be where that behaviour originated). |
|
<CustomSelectControl
inlineLabel={true}
/>I think the value disappearing thing could be considered part of the inline label functionality. What do you think would be a good name/API for this functionality? |
Jinksi
changed the title
@haszari I think After discovering that a Gutenberg FYI @mordeth @oaratovskyi the v2 of Gutenberg's const UncontrolledCustomSelect = () => (
<CustomSelect label="Colors">
<CustomSelectItem value="Blue">
{ /* The `defaultValue` since it wasn't defined */ }
<span style={ { color: 'blue' } }>Blue</span>
</CustomSelectItem>
<CustomSelectItem value="Purple">
<span style={ { color: 'purple' } }>Purple</span>
</CustomSelectItem>
<CustomSelectItem value="Pink">
<span style={ { color: 'deeppink' } }>Pink</span>
</CustomSelectItem>
</CustomSelect>
); |
…ence from `CustomSelectControl`
Jinksi
changed the title
InlineLabelSelect component for WCPay admin filter select inputs (with inline label)
|
@nagpai I've requested your review here, as your PR #8910 will using this component directly. This discussion has dived into the trade-offs, differences and approaches of the Gutenberg & WooPayments components. This has been resolved for the most part – we can now focus on reviewing this to see if it is fit for the immediate project needs: |
Resolves #8788
Changes proposed in this Pull Request
This PR introduces a new currency select component as part of the new UI for selecting currency on the Payment Overview page #8765. This PR does not make any changes to the existing UI, only introduces a new component.
Design.
Note, the placement of the inputs within the page is for demonstration only.
InlineLabelSelectcomponentclient/components/custom-select-controlcomponent (used for onboarding screens)CustomSelectControlleverages the existing logic, styles and a11y considerations from the GutenbergCustomSelectControlcomponentI've kept the API (props, interfaces) as close to the existing
CustomSelectControlas practical, to allow us to adopt this for onboarding and more in the future. Differences from the existingCustomSelectControlcomponent:Tip
I've created code diffs to illustrate what has changed between the Gutenberg → WooPayments
CustomSelectControl→InlineLabelSelect:Firstly, the diff between the Gutenberg (JSX) → WooPayments
CustomSelectControl(TSX)Secondly, the diff between the WooPayments
CustomSelectControl→InlineLabelSelect(this PR)filter.select.component.demo.mov
Note, the placement of the inputs within the page is for demonstration only.
currency.select.component.demo.mov
Future
Future requirements related to this component that are not included in this initial PR:
GroupedSelectControlcomponent.TODO
Feedback from designwill be handled via Reorganise payments overview currency switcher, welcome message and balances card #8791Testing instructions
Option A: Checkout PR #8791 which uses this component for the Payments Overview currency select
Option B: View the site Helix WooExpress site, which has PR #8791 active – find this component in use for the Payments Overview currency select.
Option C: View the demo components from the screenshot above by replacing
client/components/welcome/index.tsxwith the following code:`client/components/welcome/index.tsx`
npm run changelogto add a changelog file, choosepatchto leave it empty if the change is not significant. You can add multiple changelog files in one PR by running this command a few times.Post merge