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.

Sign up for GitHub

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

Jump to bottom

in-canvas-for-x width:auto instead of width:inherit #11141

Merged
merged 2 commits into from
Apr 13, 2018
Merged

in-canvas-for-x width:auto instead of width:inherit #11141

merged 2 commits into from
Apr 13, 2018

Conversation

RvWensen
Copy link
Contributor

@RvWensen RvWensen commented Apr 6, 2018

Issue fix.
Discussion can be found #11124

@ncoden ncoden requested review from ncoden and SassNinja April 6, 2018 12:17
@ncoden
Copy link
Contributor

ncoden commented Apr 6, 2018 •
edited
Loading

Hi @RvWensen,

Here are some recommentations your future PRs:

  • Use the same "format" as others PRs for your PR and commits titles: feat:, fix:, docs:, refactor:, clean:, chore:...
  • Provide a list of the changes mades with their reasons (even if you end in repeating yourself with what was said in the related issue).
  • If your PR closes an issue, said it explicitely (like "Closes #XXX" or "Fixes #XXX")
  • Create a dedicated branch for your PR, like fix/in-candas-for-x-width-11124. So you can continue working on your develop and propose others PRs without effects on this one.

@ncoden ncoden added this to the 6.5.0 milestone Apr 6, 2018
@RvWensen
Copy link
Contributor Author

RvWensen commented Apr 6, 2018

Thanks for the feedback!

@ncoden
Copy link
Contributor

ncoden commented Apr 6, 2018

Thanks for the PR (and issue)! I'll test it tonigh.

ncoden
ncoden reviewed Apr 6, 2018
View reviewed changes
scss/components/_off-canvas.scss Outdated
@@ -417,7 +417,7 @@ $breakpoint: small
height: auto;
position: static;
background: inherit;
width: inherit;
width: auto;
Copy link
Contributor

@ncoden ncoden Apr 6, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

This makes sense for width: auto, but @SassNinja was there any reasons for using inherit in the first place for properties that does not inherit their value by default (background, overflow, transition...) ?

Copy link
Contributor

Choose a reason for hiding this comment

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

poke @SassNinja

Copy link
Contributor

@ncoden ncoden Apr 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Done some research. The incanvas feature always used inherit, without explicit reasons given. Most font properties should inherit but not background, width, overflow, transition as this is not their default value. I'll replace all of them.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ncoden the in-canvas is supposed to just revert the off-canvas styles. Seems I was overusing the inherit a bit ;)
You're replacements of the properties in talk look good to me 👍

@ncoden
fix: reset all OffCanvas properties when in in-canvas mode
ca18489 
> Done some research. The incanvas feature always used `inherit`, without explicit reasons given. Most font properties should inherit but not `background`, `width`, `overflow`, `transition` as this is not their default value. I'll replace all of them.

See foundation#11141 (comment)
@ncoden
Copy link
Contributor

ncoden commented Apr 11, 2018

@RvWensen I did the same for all OffCanvas in-canvas properties. Let me know what you think about it. Poke @DanielRuf @SassNinja.

SassNinja
SassNinja approved these changes Apr 13, 2018
View reviewed changes
scss/components/_off-canvas.scss Outdated
@@ -417,7 +417,7 @@ $breakpoint: small
height: auto;
position: static;
background: inherit;
width: inherit;
width: auto;
Copy link
Contributor

Choose a reason for hiding this comment

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

@ncoden the in-canvas is supposed to just revert the off-canvas styles. Seems I was overusing the inherit a bit ;)
You're replacements of the properties in talk look good to me 👍

@SassNinja
Copy link
Contributor

lgtm 👍

The inherit was simply overused. Looks better now!

@ncoden ncoden merged commit 9331a28 into foundation:develop Apr 13, 2018
ncoden added a commit to ncoden/foundation-sites that referenced this pull request Jun 16, 2018
@RvWensen @ncoden
Use pull request foundation#11141 from RvWensen/develop for v6.5.0
e16be40 
3d4734a in-canvas-for-x width:auto instead of width:inherit
ca18489 fix: reset all OffCanvas properties when in in-canvas mode

Co-Authored-By: Nicolas Coden <nicolas@ncoden.fr>
Signed-off-by: Nicolas Coden <nicolas@ncoden.fr>
This was referenced Jun 16, 2018
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ncoden ncoden ncoden left review comments

@SassNinja SassNinja SassNinja approved these changes

Assignees
No one assigned
Labels
bug fix Off-Canvas
Projects
None yet
Milestone
6.5.0
Development

Successfully merging this pull request may close these issues.
3 participants
@RvWensen @ncoden @SassNinja