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

fix: Replace index() sass function with map-get() #1004

Merged
merged 2 commits into from
Jan 22, 2019

Conversation

remydenton
Copy link
Collaborator

@remydenton remydenton commented Dec 21, 2018

Jira

http://vjira2:8080/browse/BDS-660 (follow up to #987).

Summary

Fixes a sass syntax error in introduced as part of the button radius updates in #987

Details

@mikemai2awesome pointed out that some button corners had lost their radius, e.g.

https://boltdesignsystem-tsabjixlew.now.sh/pattern-lab/?p=components-custom-element-buttons
https://boltdesignsystem-tsabjixlew.now.sh/pattern-lab/?p=components-video-show-meta-title-and-duration
https://boltdesignsystem-tsabjixlew.now.sh/pattern-lab/?p=components-video-injection-test

screen shot 2018-12-21 at 1 17 18 pm

should be

screen shot 2018-12-21 at 1 17 28 pm

I'm stumped as to how the existing syntax worked for other buttons as clearly map-get() is the correct sass function, not index().

How to test

View the URLs above in the build branch for this PR and confirm the corners have a 3px radius.

@bolt-bot
Copy link
Collaborator

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

Copy link
Collaborator

@mikemai2awesome mikemai2awesome left a comment

Choose a reason for hiding this comment

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

Ok, good catch.

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.

I’m confused. If grabbing a border radius design token needs to be more complex than a flat variable (ie Sass Map) why don’t we have a simple bolt-border-radius Sass function + mixin that grabs the correct value from our collection of values we store in a Sass Map?... basically, are we actually solving the right problem here?

I feel like this sort of gotcha could happen again elsewhere (if we aren’t super careful) if we don’t have a simple tool to help grab the correct value, consistently.

Thoughts?

CC @mikemai2awesome @danielamorse

@remydenton
Copy link
Collaborator Author

Do we have any examples of that elsewhere? I feel like it's already bordering on over-engineered. I really think if just using the right sass function is all that's really needed.

@mikemai2awesome
Copy link
Collaborator

@remydenton @sghoweri

I see, so we should be using map like this, create a function for it:

$bolt-breakpoints: (
  xxsmall:  320px,
  xsmall:   400px,
  small:    600px,
  medium:   800px,
  large:    1000px,
  xlarge:   1200px,
  xxlarge:  1400px,
  xxxlarge: 1920px
);

@function bolt-breakpoint($name) {
  @return map-get($bolt-breakpoints, $name);
}

@sghoweri
Copy link
Contributor

sghoweri commented Jan 1, 2019

@remydenton @sghoweri

I see, so we should be using map like this, create a function for it:

$bolt-breakpoints: (
  xxsmall:  320px,
  xsmall:   400px,
  small:    600px,
  medium:   800px,
  large:    1000px,
  xlarge:   1200px,
  xxlarge:  1400px,
  xxxlarge: 1920px
);

@function bolt-breakpoint($name) {
  @return map-get($bolt-breakpoints, $name);
}

Exactly. And I agree that anything more complicated than @mikemai2awesome’s breakpoint example is over-engineered.

That said, I’d also argue that consistent implementations are also important for us to keep in mind.

For example, are we consistently organizing our design decisions via Sass Maps, exporting that Sass Map data via JSON, and providing tools like a Sass functions and utility classes to help use those design decisions? 😉

@remydenton
Copy link
Collaborator Author

Sorry it took me two weeks to get to this 😥. Anyway, hopefully my last commit is satisfactory and we can move this along. Can you have a look @sghoweri?

@sghoweri
Copy link
Contributor

Sorry it took me two weeks to get to this 😥. Anyway, hopefully my last commit is satisfactory and we can move this along. Can you have a look @sghoweri?

No worries @remydenton! That last update you pushed up looks good to me 🙂

If / when the need arises to move this helper function up a level to Bolt Core, most of the work is already done. Thanks!

@sghoweri sghoweri merged commit 6e9404e into master Jan 22, 2019
@sghoweri sghoweri deleted the fix/BDS-660-rounded-corner-sass-syntax branch January 22, 2019 12:32
This was referenced Feb 1, 2019
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.

4 participants