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

Breakpoint specific sizes for off-canvas #10428

Merged
merged 2 commits into from
Jul 21, 2017

Conversation

SassNinja
Copy link
Contributor

This PR add the possibility to define a different off-canvas size for small and medium by replacing the number with a map.

In a current project the off-canvas is supposed to have clearly more width for medium than for small.
But no matter what value I choose (even percentaged or viewport related), it doesn't look good for both breakpoints.
The bottom line is I need two different off-canvas sizes.

The new maps $offcanvas-sizes and $offcanvas-vertical-sizes replace the prior $offcanvas-size and $offcanvas-vertical-size. Since I don't consider any custom setting of these deprecated variables I've added warnings that appear during the gulp process.

Alternatively I could leave the variables name untouched and add several conditions to check if it's a number or a map. But imo a warning is better because it isn't much work for the user to update the settings and keeps the code cleaner.

@brettsmason @IamManchanda what do you think about this feature?

@kball
Copy link
Contributor

kball commented Jul 19, 2017

For backwards compatibility with old settings files, can you have the old variables set up the new ones if they're defined? Can also print a deprecation warning, but it's best if we don't break projects that have the older settings and not the newer.

…arning)

This preserves backwards compatibility.
@SassNinja
Copy link
Contributor Author

@kball a deprecation warning was already included by me but you're right, still supporting the deprecated settings would not be amiss.

I've updated the PR and take over the values now.

@kball
Copy link
Contributor

kball commented Jul 21, 2017

Awesome... this is looking good as far as it goes - makes me wonder though if we should make the logic iterate through the sizes (to allow someone to do this for any arbitrary map of breakpoints) rather than explicitly call out small & medium... thoughts?

Also @brettsmason would love your thoughts & review

@IamManchanda IamManchanda self-requested a review July 21, 2017 15:46
@IamManchanda
Copy link
Contributor

This is looking cool from outset... let me dig in...

$offcanvas-vertical-size: 250px;
$offcanvas-sizes: (
small: 250px,
medium: 350px,
Copy link
Contributor

Choose a reason for hiding this comment

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

Settings.scss is automatically generated! Remove this file changes

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah though it's kind of find - it will get overridden anyway.

Copy link
Contributor

@IamManchanda IamManchanda left a comment

Choose a reason for hiding this comment

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

Requested one change above ... apart that looks awesome after digging in!

Have tested it out and can confirm that this works great!
Awesome @SassNinja

@kball
Copy link
Contributor

kball commented Jul 21, 2017

I'm going to merge this as is, because I'd like to get a point release out and want to land on testable set of changes... that said, would really love a new PR that iterates over the breakpoints to allow any set rather than hard coding small & medium

@kball kball merged commit 2c6ea0d into foundation:develop Jul 21, 2017
kball added a commit that referenced this pull request Jul 25, 2017
@SassNinja SassNinja deleted the feature/offcanvas-sizes-map branch January 25, 2018 08:18
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