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

fix: mark placeholder option as selected in react SfSelect #3263

Merged

Conversation

lsliwaradioluz
Copy link
Contributor

@lsliwaradioluz lsliwaradioluz commented Jan 17, 2025

Related issue

Closes ES-1423

Scope of work

React

Added the selected attribute to the option used as the native's <select /> element placeholder since this is the option we want to use in case there is no value set initially. See example usage of an <SfSelect /> element featuring months of the year (01-12).

Before
404329437-eb8bc08a-4c80-49cd-ae45-725a1895af32

After

404329476-3e65ccfe-825b-4a95-9348-c250ffc9903f

Keep in mind the change does not interfere with our ability to set the initial value for the select element. If we do, the placeholder option will not appear as selected.

Vue

Added the internalValue ref which tracks the current value of the native <select /> element and prevents losing it in between re-renders. The erroneous behaviour can be observed in the documentation's examples - take a look at the top 3 select elements. They have no v-model applied and the selected option is not preserved due to an immediate rerender:

Screen.Recording.2025-01-17.at.15.47.23.mov

Checklist

  • Self code-reviewed
  • Changes documented
  • Semantic HTML
  • SSR-friendly
  • Caching friendly
  • a11y for WCAG 2.0 AA
  • examples created
  • blocks created
  • cypress tests created

Copy link

changeset-bot bot commented Jan 17, 2025

🦋 Changeset detected

Latest commit: 7e91c2d

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@storefront-ui/vue Patch
@storefront-ui/react Patch
@storefront-ui/nuxt Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Contributor

@Szymon-dziewonski Szymon-dziewonski left a comment

Choose a reason for hiding this comment

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

I'm not really understand issue here, if you don't have value on selected, hidden value should be set as default placeholder codepen example that is default select HTML behaviour

@@ -62,8 +60,8 @@ const modelProxy = computed({
data-testid="select"
>
<select
v-model="internalValue"
Copy link
Contributor

Choose a reason for hiding this comment

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

this wont work for sure when you will change model-value from outside, meaning

  1. when select is changed emit should be done
  2. when model is changed outside should reflect in select

Select component as any framework UI components should not keep its state, that why you should have model value outside your component and component itself is just UI representation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Select component as any framework UI components should not keep its state, that why you should have model value outside your component and component itself is just UI representation.

I thoroughly agree. Unfortunately with Vue it does not work as expected (see the documentation example above) unless we use the v-model on that component in the parent component.

Therefore, if we agree that this component should not have any internal state (which I am fine with), we should mark modelValue as required to avoid such bugs. And this will be a breaking change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@Szymon-dziewonski Szymon-dziewonski Jan 17, 2025

Choose a reason for hiding this comment

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

I would suggest using proxy with internal as it is done in SfInput

const internalState = ref<string | number>();
const selectModel= computed({
  get: () => modelValue.value ||  internalState.value,
  set: (value) => {
    emit('update:modelValue', value);
    internalState.value = value;
  },
});

Or do as you said required prop, but I see that all form control component does not have modelValue prop set as required.

So to be consistent, this component can have internal state as it is already in input (

Sorry my bad, I thought that we haven't got any component like that in SFUI.

Copy link
Contributor Author

@lsliwaradioluz lsliwaradioluz Jan 17, 2025

Choose a reason for hiding this comment

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

Thanks @Szymon-dziewonski, I've applied your suggestion

@@ -66,6 +66,7 @@ export default function SfSelect(props: SfSelectProps) {
<option
disabled
hidden
selected
Copy link
Contributor

Choose a reason for hiding this comment

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

This is breaking change as when user will add their option in slot with selected option, then user selected option wont be marked as selected because hidden placeholder is selected

Copy link
Contributor Author

@lsliwaradioluz lsliwaradioluz Jan 17, 2025

Choose a reason for hiding this comment

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

I am not sure - I believe the last option marked as selected will always take precedence.
image

Having said that, it goes against the Standard. I have to think of another solution.

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Szymon-dziewonski thank you for finding the best possible solution. Turned out using the disabled and hidden attrs simultaneously was causing this issue. I've corrected that here.

@lsliwaradioluz lsliwaradioluz force-pushed the fix/sf-select-react-placeholder-initiallu-not-selected branch from 5023f72 to 8d5d663 Compare January 17, 2025 15:38
@lsliwaradioluz lsliwaradioluz merged commit f534058 into v2-develop Jan 17, 2025
15 checks passed
@lsliwaradioluz lsliwaradioluz deleted the fix/sf-select-react-placeholder-initiallu-not-selected branch January 17, 2025 16:21
Szymon-dziewonski added a commit that referenced this pull request Jan 20, 2025
fix: mark placeholder option as selected in react SfSelect (#3263)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants