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

Use new selectmenu anatomy in explainer #798

Merged
merged 6 commits into from
Aug 22, 2023
Merged

Conversation

josepharhar
Copy link
Collaborator

This new anatomy was proposed and agreed on here:
#702

This new anatomy was proposed and agreed on here:
openui#702
@josepharhar
Copy link
Collaborator Author

@mfreed7 want to review this?

Copy link
Member

@gregwhitworth gregwhitworth left a comment

Choose a reason for hiding this comment

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

Following signoff from @mfreed7 can we also get @domenic and possibly Anne to give it a review?

site/src/pages/components/selectlist.mdx Outdated Show resolved Hide resolved
site/src/pages/components/selectlist.mdx Outdated Show resolved Hide resolved
Selectlist supports a variety of pseudo-elements to target its different parts:
- `::button` - Targets the button slot.
- `::listbox` - Targets the `<listbox>`.
- `::marker` - Targets the dropdown arrow of the default supplied button.
Copy link
Member

Choose a reason for hiding this comment

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

Are these psuedo elements only available if the author doesn't provide their own element? At which point I should be able to simply style them as elements, correct?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

They should target whatever is rendered, whether thats the default/fallback content or the author supplied one. I added a sentence to clarify

site/src/pages/components/selectlist.mdx Outdated Show resolved Hide resolved
<Part name="marker">Dropdown indicator (e.g. icon)</Part>
</Slot>
<Part name="button type=selectlist">
<Part name="selectedvalue">Currently selected value</Part>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should the part name here be selectedvalue or just value?

Reason I ask is virtually all inputs will have some form of "value" part. But selectedvalue only really makes sense for elements with multiple choices? Saying that most elements can be linked up to a datalist so maybe selectedvalue is fine for all of them?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The new <selectedvalue> element is only intended to work for <selectlist>, at least for now, and it is an element with multiple choices.

Do you think the element's tag name should be <value> instead?

Copy link
Collaborator

Choose a reason for hiding this comment

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

If it were to be value then it likely could be reused for other inputs in future?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point. I opened an issue to discuss this further: #808

@josepharhar
Copy link
Collaborator Author

can we also get @domenic and possibly Anne to give it a review?

imo they should review it on the public site after this and my remaining patches are merged to the explainer

@mfreed7
Copy link
Collaborator

mfreed7 commented Aug 19, 2023

can we also get @domenic and possibly Anne to give it a review?

imo they should review it on the public site after this and my remaining patches are merged to the explainer

+100 I really don’t like that we have so much review happening on these explainers. The feedback is awesome, and it’s not that I don’t want all of the great input! But the history and conversation on a PR is so much harder to follow later, as compared to an issue. I’d really prefer we do a very basic review on these and then land them. Fundamental (or even small) GitHub issues should then be raised to discuss changes, and the explainer gets updated after we resolve those issues. That way we have a paper trail for how we got to where we ended up.

@josepharhar josepharhar merged commit 776475b into openui:main Aug 22, 2023
4 checks passed
@josepharhar josepharhar deleted the idiom branch August 22, 2023 23:59
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.

4 participants