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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(select): unneeded top padding #1144

Merged
merged 3 commits into from Dec 7, 2021
Merged

fix(select): unneeded top padding #1144

merged 3 commits into from Dec 7, 2021

Conversation

rachelbt
Copy link
Contributor

@rachelbt rachelbt commented Dec 5, 2021

No description provided.

@github-actions
Copy link

github-actions bot commented Dec 5, 2021

馃殌

Latest successful build of the PR deployed here.

馃殌

Copy link
Contributor

@yinonov yinonov left a comment

Choose a reason for hiding this comment

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

setting label programmatically doesn't reflect on the attribute. which then causes a miss interpreted behavior;
by setting document.querySelector('vwc-select').label = '' will not change the label attribute

use mdc-select--no-label class which is set on shadowed container

@rachelbt
Copy link
Contributor Author

rachelbt commented Dec 6, 2021

mdc-select--no-label

I saw that solution in textarea. Anyway using the class is way better!
Changes as you suggested

@rachelbt rachelbt requested a review from yinonov December 6, 2021 13:55
@sonarcloud
Copy link

sonarcloud bot commented Dec 6, 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

Copy link
Contributor

@yinonov yinonov left a comment

Choose a reason for hiding this comment

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

seems stable now. wanna add to UI tests as well?

@rachelbt
Copy link
Contributor Author

rachelbt commented Dec 7, 2021

seems stable now. wanna add to UI tests as well?

I've added ui-tests in this branch:
#1133
Maybe we can merge the fix and I'll add the test there to prevent conflicts?

@rachelbt rachelbt requested a review from yinonov December 7, 2021 07:24
@rachelbt rachelbt merged commit 5ca233d into master Dec 7, 2021
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.

None yet

2 participants