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.

Sign up for GitHub

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

Jump to bottom

feat(dropdown): add experimental dropdown component #1269

Merged
merged 7 commits into from
Oct 19, 2018
Merged

feat(dropdown): add experimental dropdown component #1269

merged 7 commits into from
Oct 19, 2018

Conversation

tw15egan
Copy link
Contributor

@tw15egan tw15egan commented Oct 16, 2018 •
edited
Loading

Closes #1215

Adds in experimental Dropdown, MultiSelect, ComboBox, ListBox components

Changelog

New

  • Experimental dropdown and various extra states

Testing / Reviewing

Ensure Dropdown works properly in all states, the original drop-down is unaffected

cc @laurenmrice
Here is the staging link

@tw15egan tw15egan force-pushed the experimental-dropdown branch from 6413675 to ca6cb13 Compare October 17, 2018 16:33
@tw15egan tw15egan changed the title [WIP] feat(dropdown): add experimental dropdown component feat(dropdown): add experimental dropdown component Oct 17, 2018
@laurenmrice
Copy link
Member

@tw15egan

Browser: Firefox

For Up and Up(Light) Dropdowns:

  • There is an empty space between the drawer and the main field. Is this what you intended or to keep them flush together so they are touching? This also occurs in Chrome and Safari.
    screen shot 2018-10-18 at 10 14 16 am

  • Once you select an option, the focus border is too high on the top and comes outside of the field.
    screen shot 2018-10-18 at 10 14 27 am

For all Dropdowns:

  • The spacing of the text between the divider lines is not completely equidistant. Spacing looks correct in Chrome and Safari.
    screen shot 2018-10-18 at 10 27 14 am

alisonjoseph
alisonjoseph approved these changes Oct 18, 2018
View reviewed changes
Copy link
Member

@alisonjoseph alisonjoseph left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Code LGTM! 🎉

I did also notice the space above the dropdown on the "up" version

@tw15egan tw15egan force-pushed the experimental-dropdown branch from b1d6ef5 to a86b64e Compare October 18, 2018 16:07
@tw15egan
Copy link
Contributor Author

tw15egan commented Oct 18, 2018 •
edited
Loading

Whoops, missed the up variant 😅

Updated and pushed up a new staging site:

http://carbon-dev-environment-exp-dropdown-wise-nyala.mybluemix.net/?nav=dropdown

@laurenmrice
Copy link
Member

Yep looks good now ! 🤩👍

joshblack
joshblack suggested changes Oct 18, 2018
View reviewed changes
Copy link
Contributor

@joshblack joshblack left a comment

Choose a reason for hiding this comment

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

Looks great! Noticed a couple style quirks (potentially):

screen shot 2018-10-18 at 4 38 41 pm

In IE11, it looks like the background color bleeds down. This could be a remnant of the previous implementation, in which case might be better to file as a bug ticket that we could fix separately. Also noticed it in the disabled styles:

screen shot 2018-10-18 at 4 37 33 pm

Let me know what you think! Also had a similar note before as to whether we should do un-nesting with these components, or if we want to just leave them as is 👍

src/components/list-box/_list-box.scss Outdated
}
}

.bx--list-box__menu-item .bx--checkbox-label {
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh no! Looks like there are some lingering bx classes from the previous implementation 😬

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's see if GH "change suggestion" feature works! 😉

Suggested change
.bx--list-box__menu-item .bx--checkbox-label {
.#${prefix}--list-box__menu-item .#${prefix}--checkbox-label {

asudoh
asudoh approved these changes Oct 19, 2018
View reviewed changes
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.

LGTM 👍 from my perspective - Thanks @tw15egan!

src/components/list-box/_list-box.scss Outdated
}
}

.bx--list-box__menu-item .bx--checkbox-label {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's see if GH "change suggestion" feature works! 😉

Suggested change
.bx--list-box__menu-item .bx--checkbox-label {
.#${prefix}--list-box__menu-item .#${prefix}--checkbox-label {

@tw15egan tw15egan force-pushed the experimental-dropdown branch from d9ca34f to 1c29969 Compare October 19, 2018 15:06
@tw15egan
Copy link
Contributor Author

@joshblack unnested most selectors, kept things like focus and hover nested. Personal preference of mine 😄

I'll make a separate issue for the IE11 issue, as I'm having trouble getting the environment up and running on my machine in IE 😬

@joshblack
Copy link
Contributor

Sounds good @tw15egan! 🎉

@tw15egan tw15egan mentioned this pull request Oct 19, 2018
Closed
@alisonjoseph alisonjoseph merged commit a399e60 into carbon-design-system:master Oct 19, 2018
@carbon-bot
Copy link
Contributor

🎉 This PR is included in version 9.35.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@emyarod emyarod mentioned this pull request Feb 7, 2019
Merged
@tw15egan tw15egan deleted the experimental-dropdown branch April 28, 2021 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asudoh asudoh asudoh approved these changes

@alisonjoseph alisonjoseph alisonjoseph approved these changes

@joshblack joshblack joshblack approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@tw15egan @laurenmrice @joshblack @carbon-bot @asudoh @alisonjoseph