Skip to content

Conversation

patsissons
Copy link
Contributor

@patsissons patsissons commented Apr 12, 2019

WHY are these changes introduced?

We can currently use the page-when-not-max-width mixin to break pages as soon as they being to resize, but this is not applicable for layouts that use a frame with the nav section. For these cases we need to also take the nav width in account for the breaking width.

WHAT is this pull request doing?

This PR adds a new breakpoint variable $frame-with-nav-max-width as well as a new mixin to break before that defined width frame-with-nav-when-not-max-width().

image
image
image

🎩 checklist

The actual breakpoint doesn't line up perfectly in storybook due to the extra margins/padding involved, but the intent is still pretty clear from the screenshots above. We want the break to occur as soon as the page content begins to resize and become responsive.

Paste code into playground/Playground.tsxyarn dev → open storybook → 🎩
import * as React from 'react';

import {Banner, Frame, Navigation, Page, TopBar} from '../src';
import * as styles from './Playground.scss';

export default class Playground extends React.PureComponent {
  render() {
    return (
      <Frame navigation={<Navigation location="/" />} topBar={<TopBar />}>
        <Page title="Playground">
          <Banner>
            <p className={styles.Content} />
          </Banner>
        </Page>
      </Frame>
    );
  }
}

Requires the following Playground.scss content

.Content::after {
  content: 'Frame Max Width Page';

  @include frame-with-nav-when-not-max-width {
    content: '*** Frame Condensed Page ***';
  }
}

@BPScott BPScott temporarily deployed to polaris-react-pr-1311 April 12, 2019 17:27 Inactive
@chloerice
Copy link
Member

Hey Pat 👋Is this a WIP?

@patsissons
Copy link
Contributor Author

@chloerice not exactly. It could be ready to go (with a changelog update). But i wanted to get feedback from the Polaris team if this was something that belonged or not before pushing forward. IMO i think it belongs somewhere central, but i know that all these breakpoints are a bit of a pain point already. If you want me to get this prepped for merging i can do that!

@chloerice
Copy link
Member

chloerice commented Apr 15, 2019

Since it's a breakpoint that's dependent on the width of Polaris components, I don't see a reason not to add it here 👍.

Can you add some playground code or some other way to test that this breaks when you expect it to?

@patsissons
Copy link
Contributor Author

can do!

@patsissons patsissons force-pushed the add-variable-frame-with-nav-max-width branch from f0d4b20 to bef6bb4 Compare April 15, 2019 21:47
@BPScott BPScott temporarily deployed to polaris-react-pr-1311 April 15, 2019 21:48 Inactive
@patsissons patsissons requested a review from chloerice April 15, 2019 22:06
Copy link
Member

@chloerice chloerice left a comment

Choose a reason for hiding this comment

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

I'm not sure I'm clear on exactly when you want this to break, but it looks good to me and if you've tested the mixin in web ** and it works as you expect it to then :shipit: 👌 :shipit: 👌 :shipit:

2019-04-17 19 03 50

** You'll need to kill the web server and then run yarn run build-consumer web in a terminal tab in this polaris-react branch while in a branch of web that has the view you want to test it in. DM me if you want some help doing that 😀

@patsissons patsissons force-pushed the add-variable-frame-with-nav-max-width branch from bef6bb4 to d78272b Compare April 18, 2019 21:40
@BPScott BPScott temporarily deployed to polaris-react-pr-1311 April 18, 2019 21:40 Inactive
@patsissons
Copy link
Contributor Author

🎩 in web confirmed 👍 I will queue this up first thing Monday.

@patsissons patsissons force-pushed the add-variable-frame-with-nav-max-width branch from d78272b to d2602a7 Compare April 22, 2019 15:17
@patsissons patsissons merged commit 0023a09 into master Apr 22, 2019
@alex-page alex-page temporarily deployed to production April 22, 2019 19:26 Inactive
@kaelig kaelig deleted the add-variable-frame-with-nav-max-width branch April 26, 2019 20:55
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