Use %placeholder and @extend on button mixins to see if it makes code less repetitive #347

yvonnetangsu · 2019-03-18T21:19:13Z

READY FOR REVIEW

Summary

We will have a discussion on using @extend on some base styles when using mixins. This is a test to see if this is a good approach and what can be done differently.
I picked the button mixin to test because it's simple and get used by other components a lot, and it has a few variants.
Created a %button-base class with the button base styles that are mostly common to all button variants, and also used by some more complex components such as the CTA (uses the .su-button--big variant).
@extend this %button-base class in the variants' mixins and see if it reduces our output CSS file size and scss file size.

Needed By (Date)

Maybe take a look before Zoom meeting on Wed?

Urgency

N/A

Steps to Test

Pull this branch and run grunt styleguide
Check in the components Button, Link, CTA section that all the buttons still look and behave the same as on the live site.
Look at the files changed tab on this GitHub page and see what's deleted and what's added in the output CSS and scss styles.

Affected Projects or Products

Decanter

Associated Issues and/or People

If this get merged we'll have to make sure the card and hero components stay the same or we'll have to do some small refactor.

yvonnetangsu · 2019-03-18T21:24:18Z

Notes:

If we don't use the button mixin in other components, e.g., links, CTA and cards, we can get away with not doing @extend %button-base; in the button variant mixins (@button-big and @button-secondary). However, we do use @button-big as standalone for e.g., the CTA button, and the CTA button doesn't have a default .su-button class which has the button base styles - which is why I still include the @extend %button-base; in the button variant mixins.

sherakama · 2019-03-18T22:06:45Z

core/scss/utilities/mixins/button/_button-primary.scss


+%button-base {


I like where you are going for this. I vote for moving this to its own file and documenting it like we do mixins. Perhaps we need to create a /core/scss/utilties/placeholders/ directory?

Yeah, I think that makes sense - do you think it's better to add a directory for each component that has placeholders or just all in the root /placeholders?

Directories are good. We will likely get to a point where we will want them so let's start with them.

@sherakama , I just pushed up a commit. I created /placeholder inside /utilities. I'm using the same structure as /variables by creating a /components inside /placeholders and moved the button placeholder style there. What do you think? Since we're most likely to have only 1 placeholder for each component, I didn't create a separate /button inside /components.

TL;DR - we now have
/utilities/placeholders
/utilities/placeholders/components/_button.scss (where new %button-base is)

What do you think?

…here under /components subdirectory

sherakama

I made one more big change to this. I moved the CTA mixin to a placeholder as I feel that makes more sense than a mixin. I would say we would want to avoid complex components as mixins.

yvonnetangsu · 2019-03-19T17:32:39Z

I made one more big change to this. I moved the CTA mixin to a placeholder as I feel that makes more sense than a mixin. I would say we would want to avoid complex components as mixins.

Yeah, I think this change makes sense. I noticed that the CTA buttons aren't working right now but I'm assuming you're working on it (says fixup wip in your commit message) so I won't touch it for now. Thanks for the tweak!

josephgknox · 2019-03-19T17:57:57Z

Thank you two for you discussion and work on this ticket. Structure and strategy sound and look solid. One thing that we might want to address before merging this is:

For the CTA component: The button style is not taking. It reads "Button Text" but it is not a button. Also, for the .su-cta--button-center variant, the button is not appearing at all.

josephgknox · 2019-03-19T18:27:29Z

The button not working on CTAs appears to be an order of operations issue:

Fatal error: ".su-cta" failed to @extend "%cta-base".
The selector "%cta-base" was not found.
Use "@extend %cta-base !optional" if the extend should be able to fail.

'placeholders/index' would need to come before 'mixins/index' in the utilities rollup file, but can't because we are using mixins in placeholders.

yvonnetangsu · 2019-03-19T18:29:00Z

@josephgknox ...interesting....you found a paradox!! 😂

josephgknox · 2019-03-19T18:30:33Z

@yvonnetangsu Haha! Yep, the _cta.scss file has a typo (maybe intentional) of @extends rather than the correct @extend which is why the error was not originally being thrown. Not sure which route to go here. It would make sense to allow for mixins to be used in placeholders, but if there is a back and forth of their needing each other, I'm not sure how to proceed without refactoring...

josephgknox · 2019-03-19T18:32:14Z

@yvonnetangsu I also see you comment to Shea regarding the same/similar thing. Do we want to proceed with merging in this work, knowing that we will need to clean up the CTA buttons later?

yvonnetangsu · 2019-03-19T18:35:11Z

@yvonnetangsu Haha! Yep, the _cta.scss file has a typo (maybe intentional) of @extends rather than the correct @extend which is why the error was not originally being thrown. Not sure which route to go here. It would make sense to allow for mixins to be used in placeholders, but if there is a back and forth of their needing each other, I'm not sure how to proceed without refactoring...

Hehe..maybe this makes a case for keeping my original approach of @CTA as mixin 😄 ! Just joking - I don't think @sherakama likes this approach 🤔

@josephgknox , That's ok, I'm totally fine with this not merged in before we know that everything's working. It was sort of an experiment for discussion on Zoom meeting tomorrow anyway. So everything is open for improvement/tweaks.

josephgknox · 2019-03-19T18:37:42Z

@yvonnetangsu sounds good! More tomorrow. :)

sherakama · 2019-03-20T04:08:41Z

Sorry about the rollup mix up. I fixed the extends typo and included the cta appropriately.

Pros for this approach:

Placeholders reduce markup vs mixins
Allows for developers to add their own wrapper classes and extend the placeholder

Cons for this approach:

Adds fractioning and complexity to the code.

Let's talk more about this tomorrow.

josephgknox

G2G!

* master: Use %placeholder and @extend on button mixins to see if it makes code less repetitive (#347) Cut down on media queries generated by @modular-spacing (#343) Main Nav Light on Dark Background Color Variant (#344) Reorganize files for CTA so it's consistent with other components (#345) Update main-nav.twig (#331) get rid of ID (#342) 5 Lines Lockup + Variants (#289) Update _alert--info.scss (#330) # Conflicts: # core/css/decanter.css # core/scss/utilities/mixins/button/_button-big.scss # core/scss/utilities/mixins/button/_button-primary.scss # core/scss/utilities/mixins/button/_button-secondary.scss # core/scss/utilities/mixins/index.scss

* master: Use %placeholder and @extend on button mixins to see if it makes code less repetitive (#347) # Conflicts: # core/dist/css/decanter.css - resolved using master's, then rebuilt with webpack

* master: Refactor card component using placeholder and match Figma design (#332) Use %placeholder and @extend on button mixins to see if it makes code less repetitive (#347) # Conflicts: # core/css/decanter.css

testing using %placeholder and @extend on button component

c74ee74

yvonnetangsu changed the title ~~Use %placeholder and @extend on button component to see if it makes code less repetitive~~ Use %placeholder and @extend on button mixins to see if it makes code less repetitive Mar 18, 2019

yvonnetangsu requested review from sherakama, josephgknox and JBCSU March 18, 2019 21:30

yvonnetangsu added the enhancement label Mar 18, 2019

sherakama reviewed Mar 18, 2019

View reviewed changes

yvonnetangsu and others added 4 commits March 18, 2019 16:19

add placeholder directory in utilities directory; move %button-base t…

1b8d5d2

…here under /components subdirectory

Merge with master.

fb2c833

Moved CTA from a mixin to a placeholder.

4fe24c9

fixup! wip.

6ff9db9

sherakama approved these changes Mar 19, 2019

View reviewed changes

fixup! wip.

a839773

josephgknox approved these changes Mar 20, 2019

View reviewed changes

josephgknox merged commit 69d949f into master Mar 20, 2019

josephgknox deleted the use-extend-buttons branch March 20, 2019 18:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use %placeholder and @extend on button mixins to see if it makes code less repetitive #347

Use %placeholder and @extend on button mixins to see if it makes code less repetitive #347

yvonnetangsu commented Mar 18, 2019 •

edited

Loading

yvonnetangsu commented Mar 18, 2019 •

edited

Loading

sherakama Mar 18, 2019

yvonnetangsu Mar 18, 2019

sherakama Mar 18, 2019

yvonnetangsu Mar 18, 2019

sherakama left a comment

yvonnetangsu commented Mar 19, 2019

josephgknox commented Mar 19, 2019

josephgknox commented Mar 19, 2019 •

edited

Loading

yvonnetangsu commented Mar 19, 2019

josephgknox commented Mar 19, 2019 •

edited

Loading

josephgknox commented Mar 19, 2019

yvonnetangsu commented Mar 19, 2019 •

edited

Loading

josephgknox commented Mar 19, 2019

sherakama commented Mar 20, 2019

josephgknox left a comment

Use %placeholder and @extend on button mixins to see if it makes code less repetitive #347

Use %placeholder and @extend on button mixins to see if it makes code less repetitive #347

Conversation

yvonnetangsu commented Mar 18, 2019 • edited Loading

READY FOR REVIEW

Summary

Needed By (Date)

Urgency

Steps to Test

Affected Projects or Products

Associated Issues and/or People

yvonnetangsu commented Mar 18, 2019 • edited Loading

sherakama Mar 18, 2019

Choose a reason for hiding this comment

yvonnetangsu Mar 18, 2019

Choose a reason for hiding this comment

sherakama Mar 18, 2019

Choose a reason for hiding this comment

yvonnetangsu Mar 18, 2019

Choose a reason for hiding this comment

sherakama left a comment

Choose a reason for hiding this comment

yvonnetangsu commented Mar 19, 2019

josephgknox commented Mar 19, 2019

josephgknox commented Mar 19, 2019 • edited Loading

yvonnetangsu commented Mar 19, 2019

josephgknox commented Mar 19, 2019 • edited Loading

josephgknox commented Mar 19, 2019

yvonnetangsu commented Mar 19, 2019 • edited Loading

josephgknox commented Mar 19, 2019

sherakama commented Mar 20, 2019

josephgknox left a comment

Choose a reason for hiding this comment

yvonnetangsu commented Mar 18, 2019 •

edited

Loading

yvonnetangsu commented Mar 18, 2019 •

edited

Loading

josephgknox commented Mar 19, 2019 •

edited

Loading

josephgknox commented Mar 19, 2019 •

edited

Loading

yvonnetangsu commented Mar 19, 2019 •

edited

Loading