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

feature request: exclude invalid breakpoints instead of failing #1138

Closed
bdefore opened this issue Jan 15, 2022 · 5 comments · Fixed by #1142
Closed

feature request: exclude invalid breakpoints instead of failing #1138

bdefore opened this issue Jan 15, 2022 · 5 comments · Fixed by #1142
Labels
enhancement New feature or request

Comments

@bdefore
Copy link

bdefore commented Jan 15, 2022

I've encountered an issue where styled-breakpoints is incompatible with latest Material UI and styled-components because MUI merges its theme into the styled theme provider, including helper functions into the breakpoints key. A description of how MUI suggests working with themes for both is here: mui/material-ui#28979 (comment)

When setting up providers this way and using styled-breakpoints in a component, such as:

import { up } from 'styled-components'

const Foo = styled.div`
  ${up('sm')} {
    width: 100%;
  } 
`

The following error occurs:

Uncaught Error: [styled-breakpoints]: Check your theme. `keys: xs,sm,md,lg,xl, values: [object Object], up: function up(key) {
    var value = typeof values[key] === 'number' ? values[key] : key;
    return "@media (min-width:".concat(value).concat(unit, ")");
  }, down: function down(key) {
    var value = typeof values[key] === 'number' ? values[key] : key;
    return "@media (max-width:".concat(value - step / 100).concat(unit, ")");
  }, between: function between(start, end) {
    var endIndex = keys.indexOf(end);
    return "@media (min-width:".concat(typeof values[start] === 'number' ? values[start] : start).concat(unit, ") and ") + "(max-width:".concat((endIndex !== -1 && typeof values[keys[endIndex]] === 'number' ? values[keys[endIndex]] : end) - step / 100).concat(unit, ")");
  }, only: function only(key) {
    if (keys.indexOf(key) + 1 < keys.length) {
      return between(key, keys[keys.indexOf(key) + 1]);
    }

    return up(key);
  }, not: function not(key) {
    // handle first and last key separately, for better readability
    var keyIndex = keys.indexOf(key);

    if (keyIndex === 0) {
      return up(keys[1]);
    }

    if (keyIndex === keys.length - 1) {
      return down(keys[keyIndex]);
    }

    return between(key, keys[keys.indexOf(key) + 1]).replace('@media', '@media not all and');
  }, unit: px,` are invalid breakpoints. Use only pixels.

Note the last line about invalid breakpoints. The large output here is MUI's up helper, which styled-breakpoints accurately determines is not a valid breakpoint declaration.

This could be mitigated if the checkAllValuesHasPixels were changed to a getValidBreakpoints that filters out invalid ones determined by the check for px and proceeds with the ones that are valid instead of creating an invariant and failing. Related code: https://github.com/mg901/styled-breakpoints/blob/master/styled-breakpoints/styled-breakpoints.js#L49

Another option is to expose createStyledBreakpoints from the export on the public package so that pathToMediaQueries could be overridden, but the friendly path is to be resilient when faced with a mix of valid and invalid options.

@mg901
Copy link
Owner

mg901 commented Jan 15, 2022

Hi @bdefore I guessed that in the future there might be problems when combining with themes of third-party libraries. If I understood correctly, then the problem is that the breakpoints from the MUI theme overwrites the breakpoints from styled-breakpoints? To solve this problem i can add createTheme function. This should be enough.

styled-breakpoints.js

const defaultOptions = {
  errorPrefix: '[styled-breakpoints]: ',
-  pathToMediaQueries: 'breakpoints', 
+ pathToMediaQueries: 'styled-breakpoints.breakpoints', 
  defaultBreakpoints: {
    xs: '0px',
    sm: '576px',
    md: '768px',
    lg: '992px',
    xl: '1200px',
    xxl: '1400px',
  },
};
export const createTheme = (theme) => {
  return {
    "styled-breakpoints": {
      breakpoints: theme
    }
  };
};

app.js

import { createTheme } from 'styled-breakpoints';

const theme = createTheme({
  tablet: '600px',
  desktop: '1200px'
});

<ThemeProvider theme={theme}>
  <Component>This is cool!</Component>
</ThemeProvider>;

That do you think about it?

@bdefore
Copy link
Author

bdefore commented Jan 15, 2022

I think that'd be wise, ad-hoc namespacing your library. Would you plan for backward support for users configured for placing their breakpoints at breakpoints key? At the moment I don't know a reliable way to do that, but a major version bump and breaking change notes would be straightforward.

What you propose is good, but less of a full solution than surviving invalid values from wherever you source your breakpoints.

@mg901
Copy link
Owner

mg901 commented Jan 16, 2022

@bdefore In the next couple of days I will release a new version that will contain not only a solution to your problem, but also design inaccuracies. You can wait?

@bdefore
Copy link
Author

bdefore commented Jan 17, 2022

I can. Thanks.

@mg901
Copy link
Owner

mg901 commented Jan 19, 2022

@bdefore done!) see changes in release.

@mg901 mg901 closed this as completed Jan 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants