feature/select #70

thatsamsonkid · 2023-12-07T04:47:22Z

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our
guidelines: https://github.com/goetzrobin/spartan/blob/main/CONTRIBUTING.md#-commit-message-guidelines
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

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?

Yes
No

Other information

thatsamsonkid · 2023-12-07T04:51:38Z

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:

HLM versions of everything
Scrolling variation of select
Verify some of the form field states
Tests

goetzrobin · 2023-12-07T16:38:05Z

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!

thatsamsonkid · 2023-12-07T16:57:47Z

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?

I think to make sure someone can reasonable recreate everything with just brain

2.to make sure hlm is exposing everything it should from underlying brain and just using brain correctly overall? What do you think?

goetzrobin · 2023-12-07T21:06:58Z

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!

goetzrobin · 2024-01-02T15:37:29Z

@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 👍

thatsamsonkid · 2024-01-03T12:59:54Z

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

thatsamsonkid · 2024-01-08T16:30:44Z

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

goetzrobin · 2024-01-09T14:28:26Z

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 :)

libs/ui/select/brain/src/index.ts

elite-benni

Looks pretty good already. Left some comments.

libs/ui/select/brain/src/lib/brn-select-content.component.ts

elite-benni · 2024-01-14T06:45:17Z

libs/ui/select/brain/src/lib/brn-select-content.component.ts

+@Component({
+	selector: 'brn-select-content, hlm-select-content:not(noHlm)',
+	standalone: true,
+	imports: [BrnSelectScrollUpDirective, BrnSelectScrollDownDirective, HlmIconComponent],


A Dependency to helm should be avoided.
I am not sure how to achieve this when we want to use the same component as helm selector.

Agreed! Maybe we can work with content projection here?

That is the same problem, that I had with the switch.
When using content projection, the api is not as nice as it would be now. :/
It is already possible to use content projection, hlm Icon is used if no brnSelectScrollUp or brnSelectScrollDown is provided in the content.
So technically this is not a "tight" dependency, it would also work without Hlm.

Hmmm I had originally imagined that there would be a default icon and you would only need to provide the up/down icon arrows through projection if you wished to change the default to some other icon or icon family. I see two potential options:

A brnIcon of some sort (would this just be ngIcon lol) - probably would not be different from current hlm really and would of course make ng-icon a dep for this component so probably not the best idea but an idea none the less.

We would have to require the user to project both and up and down icon and maybe throw some warning if they don't (provide both that is) and maybe just keep a placeholder text until hlmIcon contents are provided and or simply just keep this as an optional thing. If you don't provide content we can just leave this "hover scrolling areas" out.

Or just throwing this out another idea we could keep a very simple svg inlined for the the up and down arrows as well

Let me know what you guys think or open to any other suggestions also

For me 2. sounds like the best of these options.
The other option would be to make an helm component that uses the correct content, but this has the disadvantage of adding a new layer again. :/

Yea I think lets just keep it as optional projections for these "hover scroll areas" optional. I'll make that change and i think just add some console warning if only one is provided not the other.

elite-benni · 2024-01-15T05:22:35Z

libs/ui/select/brain/src/lib/brn-select-scroll-down.directive.ts

+	standalone: true,
+	host: {
+		'aria-hidden': 'true',
+		class: 'flex cursor-default items-center justify-center py-1',


brain should not depend on tailwind

I may not need these depending on our decision on the up and down arrows. But yes will definitely remove the tw classes

libs/ui/select/helm/src/lib/hlm-select-content.directive.ts

elite-benni · 2024-01-15T06:12:42Z

libs/ui/select/helm/src/lib/hlm-select-option.component.ts

+	private readonly classNames = signal<ClassValue>('');
+
+	// eslint-disable-next-line @angular-eslint/no-input-rename
+	@Input({ alias: 'class' })


Why do you use an alias here?

libs/ui/select/helm/src/lib/hlm-select-option.component.ts

libs/ui/select/brain/src/lib/brn-select-trigger.directive.ts

goetzrobin · 2024-01-16T22:28:50Z

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!!

thatsamsonkid · 2024-01-23T05:32:28Z

thatsamsonkid · 2024-01-28T13:41:57Z

Screen.Recording.2024-01-28.at.8.38.24.AM.mov

👀👀👀

elite-benni · 2024-02-04T17:35:16Z

apps/app/src/app/pages/(components)/components/(select)/select--multiple.preview.ts

+	imports: [ReactiveFormsModule, BrnSelectImports, HlmSelectImports],
+	providers: [provideIcons({ radixChevronUp, radixChevronDown })],
+	template: `
+		<form [formGroup]="options">


Guess we can remove the form in the example now.
You want me to clean this up?

- fixes issue with root brain select component not reacting to input changes - fixes storybook issue args not being passed to component - comments out stories that still need work

1. made stuff that can be readonly readonly 2. marked things as public, protected, private explicitly 3. moved things from taps into subscribes 4. added some early returns instead of nested ifs 5. fixed a weird flickering bug when using arrows 6. fixed minor css issue with very long values 7. introduced input that allows for custom function transforming the raw value 8. introduced icon where it was possible 9. made some computedClass stuff cleaner FINALLY: HOLY SHIT IS THIS CODE A PIECE OF ART! INCREDIBLE WORK!! I AM IN LOVE WITH THIS COMPONENT

1. missing id on brn-select content 2. select not closing on making a selection in single value mode

1. remove uneeded stories and story clean up

goetzrobin · 2024-02-16T17:41:01Z

@thatsamsonkid anything I can help to get this into main? Keep up the awesome work!

thatsamsonkid · 2024-02-18T19:03:12Z

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

- adds openChanged event emitter

- add test for openChange event - add test for disabled state - add test for scroll behavior - add test for group headings accessibility attributes

docs(select): remove formcontrol from examples and preview

thatsamsonkid · 2024-02-19T23:21:41Z

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

apps/app/src/styles.css

…tyle improvements seems like contentchildren did not work the way we expected and the trigger always was undefined. we let the trigger set itself inside the service, which is accessible by the brn-select component. then we can use the reference to focus the trigger on close

Feature/select pr review

* feat(select): adds functional select brain/hlm cmps/directives * feat(label): adds label brain * feat(select): brain select component refinement - fixes issue with root brain select component not reacting to input changes - fixes storybook issue args not being passed to component - comments out stories that still need work * feat(label): brain label component * feat: adds directives for scrolling select elements * chore: select cypress init * refactor: change brn-select-content to component * refactor: adds helm selectors to brn-select, brn-select-value * feat: adds helm select directive * fix: helm select content directive with brn refactor * fix: updates select stories * fix(select): missing brn export * fix(select): incorrect setter * fix(select): removes tw class from brn-select-content * fix(select): removes unused code * fix(select): tw scroll class * feat(select): add brn-select-up/down * feat(select): add hlm select-scroll-up/down * fix(select): clear selected when multi changes from true to false * fix(select): change detection issue with selected options * feat(select): show/hide scroll up/down * fix(select): hide scrollbar * feat(select): add arrow scroll up/down * fix(select): dropdown border dark mode * feat(select): select-trigger custom icon * fix(select): option checkmark location * fix(label): add getter for id * feat(select): brn group and label * feat(select): helm select group and label * fix(select): update scrollable story * fix(select): selected options value display * fix(select): multiple value update and selection display * fix(select): service deselect all options fix * fix(select): log clean up * feat(select): sticky labels * fix(select): multi value selection indentation issue * refactor: brn-select-content * fix(select): select ada label and value reading * docs(select): add the select page with preview and examples * docs(select): use the correct formcontrolname in srcollable select * fix(select): allow select to function without explicit ngControl * fix(select): storybook cleanup * feat: minor code style improvements and a couple more. see below 1. made stuff that can be readonly readonly 2. marked things as public, protected, private explicitly 3. moved things from taps into subscribes 4. added some early returns instead of nested ifs 5. fixed a weird flickering bug when using arrows 6. fixed minor css issue with very long values 7. introduced input that allows for custom function transforming the raw value 8. introduced icon where it was possible 9. made some computedClass stuff cleaner FINALLY: HOLY SHIT IS THIS CODE A PIECE OF ART! INCREDIBLE WORK!! I AM IN LOVE WITH THIS COMPONENT * fix(select): testing surfaced issues 1. missing id on brn-select content 2. select not closing on making a selection in single value mode * fix: ts config formatting * feat(select): initial tests 1. remove uneeded stories and story clean up * docs(select): remove formcontrol from examples and preview * feat(select): support for dir - adds openChanged event emitter * feat(select): additional tests - add test for openChange event - add test for disabled state - add test for scroll behavior - add test for group headings accessibility attributes * docs: border css rule * fix: turn up and down into components and import the icons in there * fix: add border classes and allow classnames passed to hlm-select * fix: make focus return to trigger when dropdown closed & minor code style improvements seems like contentchildren did not work the way we expected and the trigger always was undefined. we let the trigger set itself inside the service, which is accessible by the brn-select component. then we can use the reference to focus the trigger on close * docs: minor improvements to select documentation & examples * fix: remove unneeded border color rule --------- Co-authored-by: elite-benni <benjamin.heiss@elite.tirol> Co-authored-by: robingotz <tug29225@temple.edu>

ajitzero mentioned this pull request Dec 30, 2023

bug: select/option in dark mode is white text on white background #96

Closed

2 tasks

thatsamsonkid marked this pull request as draft January 6, 2024 14:35

thatsamsonkid changed the title ~~(Draft) - Feature/select~~ feature/select Jan 6, 2024

thatsamsonkid force-pushed the feature/select branch from 2a65033 to 0d93c14 Compare January 6, 2024 14:41

theo-matzavinos reviewed Jan 13, 2024

View reviewed changes

libs/ui/select/brain/src/index.ts Show resolved Hide resolved

elite-benni requested changes Jan 15, 2024

View reviewed changes

thatsamsonkid force-pushed the feature/select branch from 76f7b14 to 3d4ee2a Compare January 23, 2024 05:16

thatsamsonkid force-pushed the feature/select branch 2 times, most recently from 7e56025 to 2abc152 Compare February 1, 2024 03:35

elite-benni approved these changes Feb 14, 2024

View reviewed changes

thatsamsonkid added 9 commits February 14, 2024 22:09

feat(select): adds functional select brain/hlm cmps/directives

b191adf

feat(label): adds label brain

108db4f

feat(select): brain select component refinement

e2a95ab

- fixes issue with root brain select component not reacting to input changes - fixes storybook issue args not being passed to component - comments out stories that still need work

feat(label): brain label component

9c2b610

feat: adds directives for scrolling select elements

c4f5a8a

chore: select cypress init

d48d7bb

refactor: change brn-select-content to component

53b29e3

refactor: adds helm selectors to brn-select, brn-select-value

9f3e357

feat: adds helm select directive

847f05d

thatsamsonkid and others added 11 commits February 14, 2024 22:09

fix(select): multi value selection indentation issue

67def14

refactor: brn-select-content

1d66bf3

fix(select): select ada label and value reading

8283131

docs(select): add the select page with preview and examples

f53afb6

docs(select): use the correct formcontrolname in srcollable select

b318576

fix(select): allow select to function without explicit ngControl

199c0ee

fix(select): storybook cleanup

5cd9f9d

fix(select): testing surfaced issues

4c16fbf

1. missing id on brn-select content 2. select not closing on making a selection in single value mode

fix: ts config formatting

54578a8

feat(select): initial tests

abbf382

1. remove uneeded stories and story clean up

thatsamsonkid force-pushed the feature/select branch from f6cd47f to abbf382 Compare February 15, 2024 03:09

docs(select): remove formcontrol from examples and preview

aaf4431

thatsamsonkid added 4 commits February 19, 2024 17:39

feat(select): support for dir

1615218

- adds openChanged event emitter

feat(select): additional tests

cfa2a55

- add test for openChange event - add test for disabled state - add test for scroll behavior - add test for group headings accessibility attributes

Merge pull request #6 from elite-benni/feature/select

1bddddc

docs(select): remove formcontrol from examples and preview

docs: border css rule

71faa1d

thatsamsonkid marked this pull request as ready for review February 19, 2024 23:14

thatsamsonkid commented Feb 19, 2024

View reviewed changes

apps/app/src/styles.css Outdated Show resolved Hide resolved

goetzrobin and others added 6 commits February 20, 2024 15:06

fix: turn up and down into components and import the icons in there

32fb66e

fix: add border classes and allow classnames passed to hlm-select

913ee4f

docs: minor improvements to select documentation & examples

fab9ed9

Merge pull request #7 from goetzrobin/feature/select-pr-review

bac9dd1

Feature/select pr review

fix: remove unneeded border color rule

c2d1e75

goetzrobin merged commit e071dae into goetzrobin:main Feb 21, 2024
7 of 9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature/select #70

feature/select #70

thatsamsonkid commented Dec 7, 2023 •

edited

Loading

thatsamsonkid commented Dec 7, 2023

goetzrobin commented Dec 7, 2023

thatsamsonkid commented Dec 7, 2023

goetzrobin commented Dec 7, 2023

goetzrobin commented Jan 2, 2024

thatsamsonkid commented Jan 3, 2024

thatsamsonkid commented Jan 8, 2024 •

edited

Loading

goetzrobin commented Jan 9, 2024

elite-benni left a comment

elite-benni Jan 14, 2024

goetzrobin Jan 16, 2024

elite-benni Jan 17, 2024

thatsamsonkid Jan 17, 2024 •

edited

Loading

elite-benni Jan 18, 2024

thatsamsonkid Jan 19, 2024

elite-benni Jan 15, 2024

thatsamsonkid Jan 17, 2024

elite-benni Jan 15, 2024

goetzrobin commented Jan 16, 2024

thatsamsonkid commented Jan 23, 2024 •

edited

Loading

thatsamsonkid commented Jan 28, 2024

elite-benni Feb 4, 2024

thatsamsonkid Feb 14, 2024

goetzrobin commented Feb 16, 2024

thatsamsonkid commented Feb 18, 2024

thatsamsonkid commented Feb 19, 2024

feature/select #70

feature/select #70

Conversation

thatsamsonkid commented Dec 7, 2023 • edited Loading

PR Checklist

PR Type

Which package are you modifying?

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

thatsamsonkid commented Dec 7, 2023

goetzrobin commented Dec 7, 2023

thatsamsonkid commented Dec 7, 2023

goetzrobin commented Dec 7, 2023

goetzrobin commented Jan 2, 2024

thatsamsonkid commented Jan 3, 2024

thatsamsonkid commented Jan 8, 2024 • edited Loading

goetzrobin commented Jan 9, 2024

elite-benni left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thatsamsonkid Jan 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goetzrobin commented Jan 16, 2024

thatsamsonkid commented Jan 23, 2024 • edited Loading

thatsamsonkid commented Jan 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goetzrobin commented Feb 16, 2024

thatsamsonkid commented Feb 18, 2024

thatsamsonkid commented Feb 19, 2024

thatsamsonkid commented Dec 7, 2023 •

edited

Loading

thatsamsonkid commented Jan 8, 2024 •

edited

Loading

thatsamsonkid Jan 17, 2024 •

edited

Loading

thatsamsonkid commented Jan 23, 2024 •

edited

Loading