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

Responsive props suffixes ignore casing (templateLg = templatelg) #296

Closed
dandelionadia opened this issue Feb 6, 2020 · 3 comments · Fixed by #312
Closed

Responsive props suffixes ignore casing (templateLg = templatelg) #296

dandelionadia opened this issue Feb 6, 2020 · 3 comments · Fixed by #312
Assignees
Labels
bug good first issue Good place to start for newcomers priority:medium scope:calculation This issue relates to the calculation logic

Comments

@dandelionadia
Copy link
Contributor

const templateMd = `
	contacts products links
	newsletter newsletter newsletter
	legal legal legal
`
const templateDesktop = `
contacts products links newsletter
legal legal legal legal
`
<Composition
      template={templateMobile}
      templateMd={templateMd}
      templatelg={templateDesktop}
 >...</Composition>

image

@kettanaito kettanaito changed the title ignore is it uppercase or no ( templateLg || templatelg) Responsive props suffixes ignore casing (templateLg = templatelg) Feb 6, 2020
@kettanaito
Copy link
Owner

Thanks for reporting this, @dandelionadia. This doesn't seem right.

I think that the parsePropName function that breaks down a proper name into pure prop name, breakpoint, and behavior, explicitly has i set in its regular expression, which tells it to ignore the difference in casing:

const breakpointExp = new RegExp(`(${joinedBreakpointNames})$`, 'gi')

Solution

  1. Create a unit test for parsePropName to ensure it ignores breakpoint suffixes written in lowecase.
  2. Remove the i regular expression flag.
  3. It would be nice to display a developmeny-only warning for cases like this. Whenever you have templatelg Atomic Layout should recognize you have a breakpoint suffixes in a wrong casing, and warn you "did you mean templateLg?". Such warning should be produced in development only.

@kettanaito kettanaito added good first issue Good place to start for newcomers priority:medium scope:calculation This issue relates to the calculation logic bug labels Feb 6, 2020
@kettanaito kettanaito self-assigned this Feb 7, 2020
@kettanaito
Copy link
Owner

kettanaito commented Apr 1, 2020

I've finally got to tackling this. The fix will be released in the nearest time.

Please note that if you have any responsive props with lowercase breakpoint/behavior (by mistake, I suppose), those will stop work since the next release, and you would have to rewrite them to contain capitalized breakpoint/behavior.

I also decided not to introduce any developer-focused warnings regarding this, as figuring out whether the suffix is a breakpoint or an actual part of the prop name would be extremely hard. Imagine a prop name like fantasm={true} or noxs={2}. I know those are the edge cases, but you got an idea.

@kettanaito
Copy link
Owner

The issue is fixed and published:

 - @atomic-layout/emotion@0.13.0
 - atomic-layout@0.14.0

Please update and let me know in case you experience any problems. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug good first issue Good place to start for newcomers priority:medium scope:calculation This issue relates to the calculation logic
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants