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

fix(tabs): match default spacing to spec #4487

Merged
merged 8 commits into from
Nov 19, 2019
Merged

fix(tabs): match default spacing to spec #4487

merged 8 commits into from
Nov 19, 2019

Conversation

emyarod
Copy link
Member

@emyarod emyarod commented Oct 29, 2019

Closes #4437

This PR modifies our documentation to add additional space between the tabs and tab content

@emyarod
Copy link
Member Author

emyarod commented Oct 29, 2019

should ship tab content with a class out of the box now so that we can add the spacing to the tab content container? ref #1739 and I believe there were (now missing) issues on the legacy react repo as well

cc @asudoh

@netlify
Copy link

netlify bot commented Oct 29, 2019 •
edited
Loading

Deploy preview for carbon-elements ready!

Built with commit 39f817d

https://deploy-preview-4487--carbon-elements.netlify.com

@netlify
Copy link

netlify bot commented Oct 29, 2019 •
edited
Loading

Deploy preview for carbon-components-react ready!

Built with commit 39f817d

https://deploy-preview-4487--carbon-components-react.netlify.com

@netlify
Copy link

netlify bot commented Oct 29, 2019 •
edited
Loading

Deploy preview for the-carbon-components ready!

Built with commit 39f817d

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

@emyarod emyarod force-pushed the 4437-tabs-spacing-and-type branch from 3d2c6f4 to ef4e7af Compare November 1, 2019 14:26
@emyarod emyarod marked this pull request as ready for review November 1, 2019 14:27
@emyarod emyarod requested a review from a team as a code owner November 1, 2019 14:27
@ghost ghost requested review from abbeyhrt and vpicone November 1, 2019 14:27
@emyarod emyarod changed the title docs(tabs): adjust content spacing fix(tabs): match default spacing to spec Nov 1, 2019
@emyarod emyarod requested review from laurenmrice and joshblack and removed request for vpicone and abbeyhrt November 1, 2019 14:27
@emyarod
Copy link
Member Author

emyarod commented Nov 1, 2019

since we are shipping the tab content container it makes sense for us to ship accompanying styles to space out tab content according to spec

asudoh
asudoh approved these changes Nov 1, 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 @emyarod!

joshblack
joshblack suggested changes Nov 6, 2019
View reviewed changes
packages/components/demo/js/components/ComponentExample/component-overrides.scss
min-width: 100%;

@include breakpoint('42rem') {
min-width: 720px;
}
}

.bx--tabs + div {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would we want to keep this in but add a note to remove in v11? This seems like it could break certain layouts that may rely on this behavior currently.

Copy link
Member Author

Choose a reason for hiding this comment

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

this should be demo only code, unless you are referring to something else?

@emyarod emyarod force-pushed the 4437-tabs-spacing-and-type branch from ef4e7af to c9777a3 Compare November 6, 2019 14:48
laurenmrice
laurenmrice requested changes Nov 6, 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.

React:
Default tabs

  • The third tab has content text that jumps up, should be 16px down.

Screen Shot 2019-11-06 at 1 03 24 PM

Fixed tabs

  • We need to align the start of the content text vertically with the first tab text. Right now it is too far to the right

Screen Shot 2019-11-06 at 1 04 10 PM

Vanilla:
Default and Fixed
-Placement looks correct, the content text just needs to be body-long-01.

@emyarod emyarod force-pushed the 4437-tabs-spacing-and-type branch from c9777a3 to 1478acb Compare November 8, 2019 15:45
@emyarod emyarod force-pushed the 4437-tabs-spacing-and-type branch 3 times, most recently from f4afdb9 to f9f6fb8 Compare November 12, 2019 13:29
@emyarod emyarod force-pushed the 4437-tabs-spacing-and-type branch from f9f6fb8 to 32a71d7 Compare November 15, 2019 14:16
@joshblack
Copy link
Contributor

bump @laurenmrice if you have a chance to re-review

laurenmrice
laurenmrice approved these changes Nov 18, 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 great thank you ! 👍

@asudoh asudoh requested a review from a team as a code owner November 19, 2019 07:16
@ghost ghost requested review from abbeyhrt and alisonjoseph November 19, 2019 07:16
@alisonjoseph alisonjoseph removed their request for review November 19, 2019 16:13
@joshblack joshblack merged commit ca92b34 into carbon-design-system:master Nov 19, 2019
@emyarod emyarod deleted the 4437-tabs-spacing-and-type branch November 20, 2019 05:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@laurenmrice laurenmrice laurenmrice approved these changes

@abbeyhrt abbeyhrt abbeyhrt approved these changes

@joshblack joshblack joshblack approved these changes

@asudoh asudoh asudoh approved these changes

Assignees
No one assigned
Labels
status: ready to merge 🎉
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[tabs] incorrect spacing between tab header and tab content
5 participants
@emyarod @joshblack @asudoh @abbeyhrt @laurenmrice