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

fix: replace hard coded bx prefix with variable #1381

Merged
merged 5 commits into from
Nov 2, 2018
Merged

fix: replace hard coded bx prefix with variable #1381

merged 5 commits into from
Nov 2, 2018

Conversation

alisonjoseph
Copy link
Member

@alisonjoseph alisonjoseph commented Nov 2, 2018

Noticed some hard coded .bx-- classnames. Did a find on the entire project and replaced them with #{$prefix}--

@alisonjoseph alisonjoseph changed the title Missing prefix fix: replace hard coded bx prefix with variable Nov 2, 2018
Copy link
Contributor

@asudoh asudoh left a comment

Choose a reason for hiding this comment

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

Thanks @alisonjoseph for spotting these!!

Copy link
Collaborator

@tw15egan tw15egan left a comment

Choose a reason for hiding this comment

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

Small comments that can be addressed quickly here or another PR, but other than that looks great! Thanks for catching these 👍

@@ -11,7 +11,7 @@
@import '../list-box/list-box';

@include exports('combo-box') {
.bx--combo-box > .bx--list-box__field {
.#{$prefix}--combo-box > .#{$prefix}--list-box__field {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Somewhat related to your PR, but I noticed that ComboBox has an extra box-shadow that could be bundled in with these changes

screen shot 2018-11-02 at 10 24 15 am

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like there is box shadow and also the border-bottom on the input inside the list box? 🤔

@@ -13,11 +13,11 @@
@import '../list-box/list-box';

@include exports('multi-select') {
.bx--multi-select.bx--combo-box > .bx--list-box__field {
.#{$prefix}--multi-select.#{$prefix}--combo-box > .#{$prefix}--list-box__field {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm also noticing that box-shadow here, but it looks like we never added the gray-50 border bottom. Perhaps this is a bigger change than I thought

Copy link
Member Author

Choose a reason for hiding this comment

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

Might be better in a separate issue around list box styles, there is a lot going on here 🙃

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I'll work on that this morning and fix that up. Disregard!

@@ -145,7 +145,7 @@
}

.#{$prefix}--overflow-menu-options__option--danger {
.bx--overflow-menu-options__btn {
.#{$prefix}--overflow-menu-options__btn {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure if this has always been the case, but the overflow-menu doesn't show up in experimental mode

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like this is related to product switcher? We can fix this when we build out the experimental overflow menu.
screen shot 2018-11-02 at 10 47 45 am

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah I can work on that one next week and get it figured out 👍

@tw15egan tw15egan merged commit b431033 into carbon-design-system:master Nov 2, 2018
@alisonjoseph alisonjoseph deleted the missing-prefix branch November 2, 2018 16:01
@carbon-bot
Copy link
Contributor

🎉 This PR is included in version 9.43.5 🎉

The release is available on:

Your semantic-release bot 📦🚀

joshblack pushed a commit to joshblack/carbon that referenced this pull request May 2, 2019
…arbon-design-system#1381)

* fix(Components): add initial state and remove empty state validation

* fix(Components): revert Tooltip.js changes

* fix(Components): set initial selected in TileGroup component
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