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

Support row_gutter in IE #919

Merged
merged 4 commits into from
Nov 5, 2018
Merged

Conversation

remydenton
Copy link
Collaborator

@remydenton remydenton commented Oct 18, 2018

Jira

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

Summary

Adds support for the grid component's row_gutter parameter in IE 11.
Adds demos for grid component row_gutter and vinset

Details

Notes:

  • I did not add CSS for row_gutter=none since it works by default.

Questions that should be resolved before merging:

  • Is it correct to use the $bolt-spacing-values variable for the scss loop?
  • vinset doesn't appear to work as I would have expected in the examples (pattern-lab/?p=components-grid-row-vinset-variations). Namely, It only adds an inset to the bottom of the grid. Am I doing something wrong or is this a separate bug?

How to test

  • On the this branch, go to /pattern-lab/?p=components-grid-row-gutter-variations
  • Confirm that the row gutter have parity between IE-11 and other modern browsers

@remydenton remydenton requested a review from sghoweri October 18, 2018 14:12
@bolt-bot
Copy link
Collaborator

⚡ PR built on Travis and deployed a now preview here:

@sghoweri
Copy link
Contributor

vinset doesn't appear to work as I would have expected in the examples (pattern-lab/?p=components-grid-row-vinset-variations). Namely, It only adds an inset to the bottom of the grid. Am I doing something wrong or is this a separate bug?

@remydenton try updating your PL examples to have each of your <bolt-grid-item>'s have a row-start of 2 instead of 1

@sghoweri
Copy link
Contributor

Is it correct to use the $bolt-spacing-values variable for the scss loop?

@remydenton since we need to loop through all the spacing size options and the values of those options are 1:1 here, I'd say yes it is correct.

If we needed to add or adjust which values are in this Sass map then instead I'd suggest creating a new local variable that takes the global spacing sizes and changes things out as needed.

Make sense?

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 check out my comments and make sure this all makes sense and answers your couple points you brought up.

This looks good to me otherwise!

@remydenton
Copy link
Collaborator Author

remydenton commented Oct 25, 2018

@sghoweri, thanks for the responses. I'm clear on everything except for the row-start of 2 part.

Are you saying that instead of the rows being 1, 2, and 2 in the demos, they should be 2, 3, and 4? And any usage of grid that uses inset needs to know that it shouldn't use row 1 (while any usage of grid that does not have inset should use row 1)?

I'm hoping I'm just misunderstanding-- if that's true, it feels quite complex.

@remydenton
Copy link
Collaborator Author

@sghoweri, please have a look at my most recent commit.

The part that doesn't feel right is that a grid with vinset=none actually needs to have different row numbering (i.e. starting at 1 for the first row) than grids with a specified vinset (i.e. starting at 2). Thoughts?

@remydenton remydenton assigned sghoweri and unassigned remydenton Oct 29, 2018
@remydenton remydenton merged commit 17fc663 into master Nov 5, 2018
@remydenton remydenton deleted the feature/BDS-719-grid-row-spacing branch November 5, 2018 20:59
@remydenton remydenton mentioned this pull request Nov 5, 2018
This was referenced Nov 18, 2018
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.

3 participants