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(progress-indicator): adds 'vertical' variant #1717

Merged
merged 4 commits into from
Feb 5, 2019
Merged

feat(progress-indicator): adds 'vertical' variant #1717

merged 4 commits into from
Feb 5, 2019

Conversation

werdnanoslen
Copy link
Contributor

This adds a vertical variant to the progress-indicator component, like this:

image

Changelog

New

  • Added vertical variant to the progress-indicator component

@werdnanoslen
Copy link
Contributor Author

FYI the magic numbers used in the margins, top, etc are due to the icon's internal spacing. I tried to use only as many magic numbers as were already there, but let me know if there's a better way.

@netlify
Copy link

netlify bot commented Feb 1, 2019 •
edited
Loading

Deploy preview for the-carbon-components ready!

Built with commit 11110c7

https://deploy-preview-1717--the-carbon-components.netlify.com

@asudoh asudoh requested a review from a team February 1, 2019 00:44
@asudoh
Copy link
Contributor

asudoh commented Feb 1, 2019 •
edited
Loading

Thamks @werdnanoslen for the contribution! Set our designers as the reviewer to see if they agree the new addition. In case they agree - Could you create v10 variant as well? Thanks!

@werdnanoslen
Copy link
Contributor Author

werdnanoslen commented Feb 1, 2019 •
edited
Loading

@asudoh created. Note that I'm using the normal label instead of overflowLabel on the experimental vertical variant, as width is not necessarily a constraint.
image

laurenmrice
laurenmrice requested changes Feb 4, 2019
View reviewed changes
Copy link
Member

@laurenmrice laurenmrice 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 looks good to me other than the height of each section could be 96px instead of 48px. It needs some more breathing room.

screen shot 2019-02-04 at 4 38 55 pm

@cal-smith cal-smith mentioned this pull request Feb 4, 2019
Closed
@werdnanoslen
Copy link
Contributor Author

@laurenmrice Done, and for reference, this is the non-v10 version @ 6rem/96px min-height:
image

laurenmrice
laurenmrice approved these changes Feb 5, 2019
View reviewed changes
Copy link
Member

@laurenmrice laurenmrice 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 to me, thanks Andy!

asudoh
asudoh approved these changes Feb 5, 2019
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 👍 - Thanks @werdnanoslen!

@asudoh asudoh merged commit 0edf18b into carbon-design-system:master Feb 5, 2019
@carbon-bot
Copy link
Contributor

🎉 This PR is included in version 9.70.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@werdnanoslen werdnanoslen mentioned this pull request Feb 6, 2019
Merged
@werdnanoslen werdnanoslen deleted the progress-indicator--vertical branch February 8, 2019 20:52
joshblack pushed a commit to joshblack/carbon that referenced this pull request May 2, 2019
@emyarod
fix(DatePicker): avoid rendering extra icons for experimental DatePic…
cdfd58a 
…ker (carbon-design-system#1717)

* fix(DatePicker): avoid rendering extra icons for experimental DatePicker

* fix(DatePicker): render calendar icon for experimental single variant
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

@laurenmrice laurenmrice laurenmrice approved these changes

Assignees
No one assigned
Labels
status: visual review 🔍
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@werdnanoslen @asudoh @carbon-bot @laurenmrice