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

Mrc 4811 Checkbox + DropdownItem #5

Merged
merged 4 commits into from
Feb 22, 2024
Merged

Mrc 4811 Checkbox + DropdownItem #5

merged 4 commits into from
Feb 22, 2024

Conversation

M-Kusumgar
Copy link
Contributor

@M-Kusumgar M-Kusumgar commented Dec 14, 2023

Just isolating these to keep the PRs a good chunk to review. you wont be able to see anything in the demo yet, this is just setting up for the BaseSelect!

You may find this and coming PRs easier to review because you have already reviewed chunks of them in the past!

The checkbox component just switches based on the checked prop.

The dropdownItem component conditionally render the dropdown chevrons if the option has children and will conditionally render the checkbox if a checked prop is provided.

Note: I had to change the coverage provider to istanbul because v8 (which we use for Hint) is the one this time that is saying gibberish. istanbul works pretty well for this one!

@M-Kusumgar M-Kusumgar changed the base branch from main to setup December 14, 2023 16:12
Copy link

codecov bot commented Dec 14, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (3ce4e5a) 100.00% compared to head (33ead02) 100.00%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##              main        #5   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         2    +1     
  Lines            9        25   +16     
  Branches         1         7    +6     
=========================================
+ Hits             9        25   +16     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@M-Kusumgar M-Kusumgar marked this pull request as ready for review December 14, 2023 16:18
Copy link
Member

@r-ash r-ash left a comment

Choose a reason for hiding this comment

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

This looks great to me, all very tidy!

.vnm-checked-div > .btn.disabled, .btn:disabled {
background-color: red;
border-color: red !important;
/* border-radius: 0; */
Copy link
Member

Choose a reason for hiding this comment

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

Remove this if not used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


.vnm-checked-div > .btn.disabled, .btn:disabled {
background-color: red;
border-color: red !important;
Copy link
Member

Choose a reason for hiding this comment

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

Is this doable without the !important? I wonder if that give us some issues if we want to use this somewhere else and override the styles?

Choose a reason for hiding this comment

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

Similarly for the unchecked I guess. Is the issue that these are rendered as disabled buttons in the html and the browser/bootstrap has opinions about how to colour those? Why are the buttons disabled, and how are click events going to be handled once implemented? Are the buttons going to be enabled when behaviour is implemented, or do you just want to gain the styling which makes these look buttony without actually being working buttons (and will handle click events at the level of the enclosing div)? I'm wondering if that won't be very accessible, but not sure what you're planning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep this was for the bootstrap, it was looking a little weird in hint, i have removed the disabled tags on the buttons! the !important should not affect anything else at all because this is in a style scoped tag, i think its probably good to stop anything from overriding the styles of this component

Copy link

@EmmaLRussell EmmaLRussell left a comment

Choose a reason for hiding this comment

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

Looks good! Just see comment re disabled buttons.

@M-Kusumgar M-Kusumgar changed the base branch from setup to main January 22, 2024 13:39
@M-Kusumgar M-Kusumgar merged commit 4ffb26f into main Feb 22, 2024
3 checks passed
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.

3 participants