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

[Core] Add margin back to NonIdealState component #2544

Merged
merged 2 commits into from
Jun 20, 2018
Merged

Conversation

jkillian
Copy link
Contributor

@jkillian jkillian commented May 24, 2018

Changes proposed in this pull request:

auto horizontal margins on NonIdealStates were removed between the 3.0.0-beta.0 and 3.0.0-beta.1 releases, causing visual breaks for users which were relying on that to do centering. This PR brings that margin back.

@jkillian jkillian changed the title [core] Add margin back to NonIdealState component [Core] Add margin back to NonIdealState component May 24, 2018
@blueprint-bot
Copy link

add margin back to NonIdealState component

Preview: documentation | landing | table

@llorca
Copy link
Contributor

llorca commented May 24, 2018

@jkillian why not upgrade your NIS container to be flex instead?

@jkillian
Copy link
Contributor Author

Definitely could! Or I could just add this line of CSS to my app's CSS. I just don't want users who upgrade to BP3 to have to deal with this breaking change unnecessarily.

@llorca
Copy link
Contributor

llorca commented May 24, 2018

I guess it's more of a philosophical question. I personally would prefer Blueprint 3 to be full flex and React 16 only, but that's not possible. So just like we're in between React 15 and 16, I wonder whether it makes sense to be in between flex and non flex

@giladgray giladgray self-requested a review May 24, 2018 18:59
@giladgray
Copy link
Contributor

i only removed this line cuz it conflicted with an earlier attempt at the example frame styles. it's actually not a conflict anymore (jason and i discovered this offline).

giladgray
giladgray previously approved these changes May 24, 2018
@@ -23,6 +23,7 @@ Styleguide non-ideal-state
@include pt-flex-container(column, $pt-grid-size * 2);
align-items: center;
justify-content: center;
margin: 0 auto;
width: 100%;
max-width: $pt-grid-size * 40;
Copy link
Contributor

Choose a reason for hiding this comment

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

@jkillian 🤔 i wonder if your layout issue would be resolved by also removing the max-width instead of re-adding this margin?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm, w/o max width though, I believe description lines of text won't wrap and will be awkwardly long.

Copy link
Contributor

Choose a reason for hiding this comment

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

we can move the width to those elements rather than on the container?

Copy link
Contributor

Choose a reason for hiding this comment

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

that seems more sound to me. setting margin and width on a root element affects its layout wrt other elements, so i'd like to avoid that.

@blueprint-bot
Copy link

move max-width to children, remove margin

Preview: documentation | landing | table

@giladgray giladgray dismissed their stale review June 12, 2018 19:36

want to hear from @jkillian

Copy link
Contributor

@giladgray giladgray left a comment

Choose a reason for hiding this comment

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

merging this so it can be in RC

@giladgray giladgray merged commit 8f16f51 into develop Jun 20, 2018
@giladgray giladgray deleted the jk/nisMargin branch June 20, 2018 22:52
@jkillian
Copy link
Contributor Author

Sorry, just getting to this now. Seems reasonable to me! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants