-
-
Notifications
You must be signed in to change notification settings - Fork 75
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
New group rounded feature and small color fix control themes #49
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, thanks for the new addition of the tags. I see you're hard at work on adding some new things which are welcome. Please address the comments and I'll review again.
src/ext/tags.scss
Outdated
border-radius: 0; | ||
&.group-tags { | ||
/* Round first and last tag in the grouped tags */ | ||
&.group-rounded { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To try to stick to BEM naming conventions moving forward, we should name this group-tags--rounded
.
} | ||
|
||
&:last-child { | ||
border-radius: 0 290486px 290486px 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shorthands of length 4
are not allowed. Value was 0 290486px 290486px 0
// Round first and last tag in the grouped tags | ||
&--rounded .tag { | ||
&:first-child { | ||
border-radius: 290486px 0 0 290486px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shorthands of length 4
are not allowed. Value was 290486px 0 0 290486px
} | ||
|
||
&:last-child { | ||
border-radius: 0 0.25rem 0.25rem 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shorthands of length 4
are not allowed. Value was 0 0.25rem 0.25rem 0
&:not(:first-child):not(:last-child) { | ||
border-radius: 0; | ||
&:first-child { | ||
border-radius: 0.25rem 0 0 0.25rem; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shorthands of length 4
are not allowed. Value was 0.25rem 0 0 0.25rem
&.group-tags { | ||
.tag, | ||
&--rounded .tag { | ||
margin-right: 0 !important; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
!important should not be used
@@ -151,19 +151,33 @@ | |||
} | |||
|
|||
/* Used for grouping tags together */ | |||
&.group-tags .tag { | |||
margin-right: 0 !important; | |||
&.group-tags { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line should be indented 2 spaces, but was indented 4 spaces
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
Description
Sorry for so much PR's :D
I've added a common use case for actual rounding group tags. I think it should be pretty useful for some use cases. Furthermore, there are rounded tags so why not having grouped rounded tags ;)
In addition, I've added the primary color to the control themes. This fix resolved that primary colored closing tags are not clickable.
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
You can see the result of the new rounded grouping tags in the image below: