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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Rework logic for grid ordering in hero component #1799

Merged
merged 2 commits into from
Mar 27, 2020

Conversation

remydenton
Copy link
Collaborator

Jira

http://vjira2:8080/browse/BDS-2079

Summary

Fixes a bug where hero reverseOrder prop does not work when imageValign is bottom

Details

I set out only to fix the original bug. However, I found the existing twig logic for hero to be pretty difficult to read, in part because there were a number of logic errors. For example:

{% set _isImageFirstOnMobile = (imageValign != 'bottom') or (imageValign == 'top') %}
{% set _isImageLastOnMobile = (imageValign != 'top') or (imageValign == 'bottom') %}

First, both of these statements are logically identical if you remove the second conditions (if something is "not bottom", you don't need to also check for whether it could be "top").

{% set _isImageFirstOnMobile = (imageValign != 'bottom') %}
{% set _isImageLastOnMobile = (imageValign != 'top') %}

Digging in further, what happens if the value of imageValign is middle? Well, both of the above statements evaluate to true, so you end up with image on mobile being both first and last. 馃檭

So I ended up re-working a lot of the logic here. Hopefully it's equivalent to what was there before without the additional bugs.

How to test

Reproduce the bug

  • Check out any other branch
  • Edit docs-site/src/pages/pattern-lab/_patterns/02-components/hero/20-hero--reverse-order.twig
    Change imageValign to bottom
    View the pattern at /pattern-lab/?p=components-hero--reverse-order
    Note that the image is on the right (expected on the left).

Confirm the fix

  • Check out the branch in this PR
  • Confirm that the steps above give the expected results
  • Confirm that all other scenarios give expected results

@sghoweri
Copy link
Contributor

@remydenton yeah, I think these couple of quirks are side-effects from the original implementation allowing for desktop and mobile order to both be independently configured before getting consolidated at the 11th hour... 馃う鈥嶁檪

image

Copy link
Contributor

@sghoweri sghoweri left a comment

Choose a reason for hiding this comment

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

@remydenton see my comment about the (duplicate?) comment / variable block of code -- otherwise I'm good with these updates. Tested locally and everything still seems to be working as expected 馃憤

@sghoweri
Copy link
Contributor

@remydenton reminder: make sure you add a label based on the SEMVER release type so things get automatically published to correct version (plus add the milestone release version so we can track what work is expected to go out in which release!)

@sghoweri sghoweri added the patch label Mar 27, 2020
@sghoweri sghoweri added this to the v2.21.0 milestone Mar 27, 2020
@remydenton remydenton merged commit 6d49162 into master Mar 27, 2020
@remydenton remydenton deleted the fix/BDS-2079-hero-layout-logic branch March 27, 2020 16:36
@sghoweri
Copy link
Contributor

sghoweri commented Apr 8, 2020

PR was released with v2.21.0

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.

None yet

2 participants