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

hlmSelect does not have an invalid state style #289

Open
1 of 2 tasks
benjaminforras opened this issue May 9, 2024 · 9 comments
Open
1 of 2 tasks

hlmSelect does not have an invalid state style #289

benjaminforras opened this issue May 9, 2024 · 9 comments
Labels
bug Something isn't working

Comments

@benjaminforras
Copy link
Contributor

Please provide the environment you discovered this bug in.

Copy pasted from the components page and added ngModel with a validator.

Which area/package is the issue in?

select

Description

While Angular adds the necessary ng-invalid ng-touched classes to the host (brn-select), the helm does not provide any styling for the invalid state. Also, if wrapped in hlmLabel the label also doesn't change (based on the classes it only checks for hlmInput state).

image
image

Please provide the exception or error you saw

No response

Other information

No response

I would be willing to submit a PR to fix this issue

  • Yes
  • No
@benjaminforras benjaminforras added the bug Something isn't working label May 9, 2024
@goetzrobin
Copy link
Owner

@thatsamsonkid is this related to your most recent tests?

@thatsamsonkid
Copy link
Collaborator

@goetzrobin if i understood correctly it just the appearance of the error state for select using ng-model is not working as expected. So label is not turning red essentially. So separate issue from the other one im pretty sure.

@thatsamsonkid
Copy link
Collaborator

I will add I don't think we have support for wrapping the select in a label because label won't have access to ngControl in this case. Unless anyone has any ideas on that one. I can only think to have brnLabel check for instances of select as a contentchild in this case

@goetzrobin
Copy link
Owner

@benjaminforras does this example help:

export const ReactiveFormControlWithValidation: Story = {

I see your label seems to be missing the hlmLabel directive and you are using the brn-select instead of the helm-select component. We should probably double check the docs and add examples for template driven and reactive forms

@benjaminforras
Copy link
Contributor Author

benjaminforras commented May 21, 2024

My bad, I provided a less context to this issue that I intended.
So here's my code:

<div>
  <label class="relative" hlmLabel>
    Type
    <brn-select
      class="my-1 block w-full md:min-w-56"
      #typeInput="ngModel"
      [(ngModel)]="newEntry.type"
      [validators]="typeValidators"
      name="type"
      placeholder="Select an option">
      <hlm-select-trigger class="w-full md:min-w-56">
        <hlm-select-value />
      </hlm-select-trigger>
      <hlm-select-content class="w-full">
        @for (option of newEntryOptions; track option) {
          <hlm-option [value]="option">{{ option }}</hlm-option>
        }
      </hlm-select-content>
    </brn-select>
    <error-message [control]="typeInput" />
  </label>
</div>

With the example you've provided still does not apply the invalid styles for the select component.
The focus is not on the label, but the select element itself. While Angular applies the required ng-invalid class to the element, the element itself does not provide any styling to visualize the invalid state of the component:
image

This is for a simple text input:
image

Hope this helps! :)

TLDR: hlmSelectTrigger does not have these classes: [&.ng-invalid.ng-touched]:border-destructive [&.ng-invalid.ng-touched]:focus-visible:ring-destructive [&.ng-invalid.ng-touched]:text-destructive

more context:
  • the validators directive:
    image
  • the error-message component:
    image
    image

@thatsamsonkid
Copy link
Collaborator

@benjaminforras Ah I see unfortunately Shadcn styling doesnt actually turn the select (destructive red) by default if you want to be that way then you would need to modify to that but seems they only give label and error message the red styling for denoting an error. See the example here under select form https://ui.shadcn.com/docs/components/select

Here's a screenshot also
image

@benjaminforras
Copy link
Contributor Author

benjaminforras commented May 21, 2024

Got it, I just pointed out an inconsistency ;).

But as I see it, it does not have styling for a simple text input if we are basing it on shadcn.
image

@goetzrobin

@goetzrobin
Copy link
Owner

I think adding the styling is a nice touch and I'd like to support it. I'll take a look at it

@thatsamsonkid
Copy link
Collaborator

@goetzrobin This reminds me we need to an create an error directive also so we can easily show hide the error as well similar to mat-error and keep the styling and spacing consistent when attached to a form field

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants