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

Prop with explicit "down" behavior overrides implicit "up" behavior of preceding prop #119

Open
1 of 2 tasks
kettanaito opened this issue Feb 24, 2019 · 4 comments · May be fixed by #198 or #226
Open
1 of 2 tasks

Prop with explicit "down" behavior overrides implicit "up" behavior of preceding prop #119

kettanaito opened this issue Feb 24, 2019 · 4 comments · May be fixed by #198 or #226
Assignees
Labels
bug needs:tests This issue needs tests. Desperately. priority:high This issue violates the specification scope:calculation This issue relates to the calculation logic

Comments

@kettanaito
Copy link
Owner

kettanaito commented Feb 24, 2019

Why:

Wrong prioritization of up/down props.

Environment:

  • atomic-layout version: 0.6.2

Steps to reproduce:

Steps to reproduce the behavior:

<Composition
  templateCols="200px auto"
  templateColsSmDown="400px 1fr"
/>

On xs screens the grid-template-columns value will still be 400px 1fr (originating from templateColsSmDown.

Expected behavior:

Props with "down" behavior indeed fill any gaps below them, but when any explicit prop is met (i.e. templateCols for xs), this explicit prop must take the highest priority.

Roadmap

  • Fix
  • Add covering test
@kettanaito kettanaito added the bug label Feb 24, 2019
@kettanaito kettanaito added this to the 1.0 milestone Feb 24, 2019
@kettanaito kettanaito added the scope:calculation This issue relates to the calculation logic label Feb 26, 2019
@kettanaito kettanaito modified the milestones: 1.0, Bugs Mar 16, 2019
@kettanaito kettanaito added the priority:high This issue violates the specification label Mar 22, 2019
@kettanaito kettanaito self-assigned this Mar 23, 2019
@kettanaito kettanaito added the needs:tests This issue needs tests. Desperately. label Mar 23, 2019
@kettanaito
Copy link
Owner Author

As the library is mobile-first by default, any @media expressions take higher priority over the default (mobile) values.

.composition {
  padding: 10px;

  @media (max-width: 768px) { /* prop with a "down" behavior */
    padding: 20px; /* will be "20px" on mobile as well */
  }
}

Perhaps, listing down behavior is not compliant with the mobile-first approach. I see two ways of solving this issue:

  1. Increase specificity of default (mobile) properties. This would most likely mean some explicit selector, or logic, which I would be against.
  2. Remove defaultBehavior option and make Atomic layout mobile-first always. Alternatively, remove the "down" value of this option, leaving "up" and "only" (depends if "only" makes sense in case of responsive props application).

@kettanaito
Copy link
Owner Author

kettanaito commented May 10, 2019

It would be nice to have this fixed, yet it's likely that this API would be deprecated in the future (#165). There must be a way to resolve this CSS specificity issue without altering the library's API.

@kettanaito kettanaito removed this from the Bugs milestone Sep 6, 2019
@kettanaito
Copy link
Owner Author

kettanaito commented Sep 21, 2019

CSS-wise, there is no API to separate such two rules specificity, as the second one's will always be higher by the fact of later declaration, and the application surface. We are speaking about the scenario when the screen satisfies the media query, but the root-level property must be applied (see the beginning of the thread).

.element {
  color: red;
 
  @media (max-width: 700px) {
    // When should I stop being blue and become red?
    // There is no programatic way to tell.
    color: blue;
  }
}

That's not a CSS issue, but the issue with how a media query is constructed for the responsive props with the "down" suffix. I think such responsive props declarations should:

  1. Try to find the closest preceding sibling property;
  2. If found, prepend its breakpoint to the composed media query.

Example

Preceding sibling prop present

<Composition
  templateCols="200px auto"
  templateColsSmDown="400px 1fr"
/>

When parsing templateColsSmDown:

  1. Takes the props with the same name (templateCols) and tries to find the first sibling iterating down the list of props.
  2. Finds templateCols="200px auto" ("xs" breakpoint).
  3. Prepends breakpoints.xs.maxWidth as the min-width parameter of the media query:
/*     "xs" prefix            "sm" closed breakpoint */
@media (min-width: 500px) and (max-width: 576px) {
  grid-template-columns: 400px 1fr;
}

Preceding sibling prop missing

<Composition
  padding={10}
  templateColsSmDown="400px 1fr"
/>

When parsing templateColsSmDown:

  1. Takes the props with the same name (templateCols) and tries to find the first sibling iterating down the list of props.
  2. Finds nothing, treats templateColsSmDown as if it should apply all the way down.
  3. Generates the following media query:
@media (max-width: 576px) {
  grid-template-columns: 400px 1fr;
}

Technical aspect

Such changes may require to refactor the way applyStyles works. If the sibling props are grouped first, then it would be possible to analyze them to determine whether responsive props with "down" behavior should be closed.

@kettanaito kettanaito linked a pull request Sep 22, 2019 that will close this issue
5 tasks
@kettanaito
Copy link
Owner Author

Due to the amount of possible variations in responsive props declaration, I suggest the solution to this problem would be as simple as possible:

  • Whenever a "down" responsive prop is met, it iterates down (or straight jumps to the first record, if it's not the first one already) in its records and when it finds a record with no explicit breakpoint (treating this as the default breakpoint), and encloses the media query of "down" responsive prop according to that found default breakpoint record.

All the other use cases are rather abstract and not likely to appear in real world usage. I wouldn't invest time and library's size into figuring out how to treat them until a real necessity arises.

@kettanaito kettanaito changed the title Prop with explicit "down" behavior overrides explicit "up" behavior of preceding prop Prop with explicit "down" behavior overrides implicit "up" behavior of preceding prop Nov 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug needs:tests This issue needs tests. Desperately. priority:high This issue violates the specification scope:calculation This issue relates to the calculation logic
Projects
None yet
1 participant