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

feat: allow easy customizing of required indicator's color #2810

Merged
merged 1 commit into from
Oct 11, 2021

Conversation

knoobie
Copy link
Contributor

@knoobie knoobie commented Oct 8, 2021

Description

Allows easy customization of the default required indicator's color to allow for common use cases like red colored required indicator without overwriting the part=required-indicator for every component.

Type of change

  • Bugfix
  • Feature

Checklist

  • I have read the contribution guide: https://vaadin.com/docs-beta/latest/guide/contributing/overview/
  • I have added a description following the guideline.
  • The issue is created in the corresponding repository and I have referenced it.
  • I have added tests to ensure my change is effective and works as intended.
  • New and existing tests are passing locally with my change.
  • I have performed self-review and corrected misspellings.

Additional for Feature type of change

  • Enhancement / new feature was discussed in a corresponding GitHub issue and Acceptance Criteria were created.

--> Half / Half .. it was dicussed with Jouni in the ticket; but there is not AC created.

@knoobie
Copy link
Contributor Author

knoobie commented Oct 8, 2021

@web-padawan Sorry, but I'm kinda lost how to create a test for this change.. the whole JS is a little bit to magical for me.. I looked for a test that checks that the other variable (--lumo-required-field-indicator) works but could not find one to base my test on it.

@web-padawan
Copy link
Member

@knoobie Thanks for the PR. No worries, we can help with adding tests for this on Monday.
We don't have visual tests for Lumo package yet, so I will ask Jouni about how to proceed.

@jouni
Copy link
Member

jouni commented Oct 11, 2021

I don't remember any more if we’ve discussed this previously, but I feel we could/should drop the opacity property from the indicator, and let it be shown for both non-filled and filled fields.

@web-padawan
Copy link
Member

Yes we discussed it: #85 but let's make sure we land that in a separate PR, it will require adding some visual tests.

@knoobie knoobie changed the title feat: allow easy customizing of required indicator's color and opacity feat: allow easy customizing of required indicator's color Oct 11, 2021
@knoobie
Copy link
Contributor Author

knoobie commented Oct 11, 2021

Updated PR and removed the opacity change

@sonarcloud
Copy link

sonarcloud bot commented Oct 11, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@web-padawan
Copy link
Member

The PR looks good to me. I'm going to merge it as is, then we can think of adding visual tests to Lumo separately.

@vaadin-bot
Copy link
Collaborator

This ticket/PR has been released with platform 22.0.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants