Closed thatsamsonkid closed 6 months ago
Wanted to open a draft PR to get some initial feedback on what I have so far, so yea let me know what you think Still pending:
Awesome! This is a great start! How deep do you want the feedback to go? Just overall if this is going into the right direction or is there any more specific things you'd like me to address?
Thanks for working on this! I am super excited for this!
I think overall/high-level for now. Im still tidying up everything. Can definitely do a more in depth one once I think this is closer to being done.
I think one thing I also wanted an opinion on is do you think we should have stories for just brain usage and one with only hlm?
2.to make sure hlm is exposing everything it should from underlying brain and just using brain correctly overall? What do you think?
So one thing we decided to do going forward is to allow him to depend on brain directly. This means him can use brain components in their own template or via host directives, etc.
However, brain should never depend on helm and we should expose enough information so you can build your own UI lib just with brain components.
Radix UI = Brain shadcn = Helm
We are implementing both at the same time so we need to be a little more careful, but overall brain should do all the work and him only does some minor additions, adds styles, and improves API surface for those using them together!
Overall, I really like how this is coming along though! I think you are absolutely on the right track!
@thatsamsonkid let me know if I can do anything else to help or if you got everything you need for this! This is going to be a great addition 👍
Definitely!, I think I was getting hung up on some things for it and just needed a small break from it 😅. Let me verify I pushed everything I have so far and maybe I can get help with some specific things
Ok I updated this branch with latest and wanted to give an overview of what is remaining for this
So remaining items/issues:
Determining minimum number of items to show. Should we have a default. Configurable number. Im a little unsure how radix/shadcn is determining whether to show few options or if you see their "scrolling" example its showing almost as many as possible it seems.But seemingly without any like direct input - need some consensus on this should work i guess
Need to fix overlay cdk to open above or below depending on space - could use assistance on this or an example could help me also
Showing and hiding the scrolling arrows needs to be fixed mainly an issue with where and how Im setting the dropdown scroll
Tests for Select
Minor issue with selected option showing as checked. Is only updating after hovering over old option
Group heading and section directive styling still needs to be worked on
@goetzrobin
Sweet, I'll try to check this out this week! @elite-benni feel free to also take a look at this if you have time :)
Agree with @elite-benni's comments! I'll need to find some time to give this the attention it deserves! Thanks for your great work on this @thatsamsonkid! Probably one of the hardest and most impactful primitives! I think we are pretty close on this!!
Alrighty update on changes, seems never ending for some reason 🥲
Completed Items/fixes:
[x] Showing and hiding the scrolling arrows needs to be fixed mainly an issue with where and how Im setting the dropdown scroll
[x] Minor issue with selected option showing as checked. Is only updating after hovering over old option
[x] Fixed the selections so if multiple selections are made and mutilple value goes from true -> false then selections are cleared. (Turns out cdkListbox does this by default as well so we're on the right track there)
[x] Need to fix overlay cdk to open above or below depending on space - could use assistance on this or an example could help me also
[x] While I fixed the up/down arrow show hide and made some select "viewport" updates, still needs to be tweaked slightly with up/down arrows covering very first and last options
[x] still need some input on how we should determine/configure number of items to show at once by select, not only for default but also for scrollable.
[x] In dark mode the select dropdown out line is a bright white. Not entirely sure why
[x] Optional group heading sticky
[x] Group heading and section directive styling still needs to be worked on
[x] Check browser autofill behavior (will resolve as part of combobox/command rework)
[x] dir support
[x] Tests for Select
[x] Select Docs
@goetzrobin @elite-benni
@thatsamsonkid anything I can help to get this into main? Keep up the awesome work!
I should have some time tomorrow to hopefully wrap up the tests.
Mainly just need to add tests for some of the keyboard actions and scrolling. Then refine docs maybe. I was thinking I should probably add a few useful event outputs like on open/close to start with.
I think autofill is still an issue here but I think I will resolve that once I help convert combobox and command to our own implementation since its very similar.
@goetzrobin
All, I think this might be ready for serious consideration to merge finally!. Do we need to test cli/nx setup? I think docs we can do more but I know we were still looking into how to best format inputs/outputs of the components.
But for now the same level of docs seems to be there now thanks @elite-benni and I think I got a good number of tests there to test the core functionality!
Let me know if anything
@goetzrobin @elite-benni
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
Which package are you modifying?
What is the current behavior?
Closes #
Select Primitive
Closes #65
What is the new behavior?
Does this PR introduce a breaking change?
Other information