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): added inline dropdown, style changes to dropdown, fixed link in data table readme #732

Closed
wants to merge 14 commits into from
Closed

feat(dropdown): added inline dropdown, style changes to dropdown, fixed link in data table readme #732

wants to merge 14 commits into from

Conversation

marijohannessen
Copy link
Contributor

Overview

Closes #694
Closes #200

  • Added Inline Dropdown as this was missing from the vanilla repo (only available as the Multi-select component)
  • Made some style changes to the Dropdown component to match the Design Kit
  • Changed the filename of Inline Select so it shows up in the dev environment
  • Added in a link in the Data Tables README

Added

  • dropdown--inline.html

Changed

  • inline-select.html to select--inline.html
  • README.md for data-table-v2

Testing / Reviewing

Make sure inline Dropdown works as expected.

marijohannessen and others added 8 commits April 18, 2018 14:30
@marijohannessen
[WIP] feat(inputs): new inputs (#710)
b740252 
* feat(text-input): started working on new styles

* feat(text-inputs): added new variations of text inputs

* feat(text-inputs): added new styles to text area

* feat(text-inputs): added new styles to date picker

* feat(inputs): finished up first iteration of inptus
@tw15egan
feat(color): Update colors (#711)
451e34b 
* feat(color): start updating colors

* feat(color): update more colors

* feat(color): update hover colors

* feat(color): tweak some more colors

* feat(color): update date-picker colors

* feat(color): update table colors
@marijohannessen
[WIP] feat(inputs): new inputs (#710)
e7ec143 
* feat(text-input): started working on new styles

* feat(text-inputs): added new variations of text inputs

* feat(text-inputs): added new styles to text area

* feat(text-inputs): added new styles to date picker

* feat(inputs): finished up first iteration of inptus
@tw15egan @marijohannessen
feat(color): Update colors (#711)
ce5dfa3 
* feat(color): start updating colors

* feat(color): update more colors

* feat(color): update hover colors

* feat(color): tweak some more colors

* feat(color): update date-picker colors

* feat(color): update table colors
@marijohannessen
fix(global): updated branch
3ee2e9c
@marijohannessen
Merge branch 'master' into beta
2429fb1
@marijohannessen
feat(form): added v2 version of form
e5155a9
@marijohannessen
Merge branch 'beta' of github.com:carbon-design-system/carbon-compone…
e676206 
…nts into beta
@marijohannessen marijohannessen changed the title feat(dropdown): added inline dropdown feat(dropdown): added inline dropdown, style changes to dropdown, fixed link in data table readme Apr 27, 2018
asudoh
asudoh reviewed Apr 27, 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.

Huge thanks @marijohannessen for introducing this variant! I’m excited when I saw the one in our dev env! A couple of questions:

  • Is the style changes to non-inline dropdown intentional?
  • Should the darkened hover over be shrunk to the length of text?

alisonjoseph
alisonjoseph approved these changes May 1, 2018
View reviewed changes
Copy link
Member

@alisonjoseph alisonjoseph 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, the only question I have is about the hover/focus state width? Should it be 100% width? Or only as wide as the text/carat? Looks like this might be an existing issue, I checked the react storybook and it looks the same.
screen shot 2018-05-01 at 10 31 24 am

@marijohannessen
Copy link
Contributor Author

@asudoh yes, the style changes are intentional 🙂
@alisonjoseph I noticed that too, but I'm not sure if I can get it to just hover over the text and keep it accessible. I'll give it a try though!

@marijohannessen
Copy link
Contributor Author

Alright, was able to fix it @asudoh and @alisonjoseph ! 🙂

@alisonjoseph
Copy link
Member

@marijohannessen If you choose an option now the arrow disappears. 😕
screen shot 2018-05-02 at 11 49 23 am

@marijohannessen
Copy link
Contributor Author

Oh no! Will fix! 🙂

alisonjoseph
alisonjoseph approved these changes May 2, 2018
View reviewed changes
Copy link
Member

@alisonjoseph alisonjoseph 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!

asudoh
asudoh reviewed May 3, 2018
View reviewed changes
Copy link
Contributor

@asudoh asudoh 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.

It’s great that the trigger button is now shrunk to the size of text, @marijohannessen! 🎉 It’d be great if the JavaScript code doesn’t have to take care of the arrow icon - I put a possible approach to: asudoh@879187f

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asudoh asudoh asudoh left review comments

@alisonjoseph alisonjoseph alisonjoseph 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.
4 participants
@marijohannessen @alisonjoseph @asudoh @tw15egan