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

Variables File #158

Open
patrickmckowen opened this issue Jun 15, 2016 · 17 comments
Open

Variables File #158

patrickmckowen opened this issue Jun 15, 2016 · 17 comments
Assignees
Milestone

Comments

@patrickmckowen
Copy link
Contributor

I'd like to discuss removing the @import "scss/variables"; and have all variables in our file.

The benefits I see:

  • Removes the need to compare two different files to get the full picture of the DS's variables.
  • Turns our variables file into a Single Source of Truth for JDRF styles. If a vendor is not able to use our HTML for whatever reason, we can at least provide them our variables file to get a baseline level of branding.

If Bootstrap v4.5 or v5 has a bunch of cool things in their variables file, we would need to spend time evaluating it before adopting anyway, so I don't see a ton of efficiency gained by importing the Bootstrap file.

Thoughts?

@RachelRVasquez @fuhton @briansaycocie

@fuhton
Copy link
Contributor

fuhton commented Jun 15, 2016

This makes sense to me. @RachelRVasquez Thoughts?

@RachelRVasquez
Copy link
Contributor

@fuhton My only concern is since Bootstrap is being pulled from node_modules, if there's ever a Bootstrap upgrade and variables change/get removed/added, our custom variable file would have to be reviewed every time. Since Bootstrap relies so heavily on variables, there's a potential the DS will break with future upgrades.

From reviewing the variables file as well - about half of them are overwrites whereas the other half are custom variables we added to imitate Bootstrap's structure for DS. Compared to the amount of variables in Bootstrap, I don't think we're at the point just yet where we're overwriting more variables than Bootstrap's original file. I'd like to only remove imports when we're overwriting Bootstrap's original file so much that it no longer makes sense to keep the import due to overwriting most of it. This is the route we took with the buttons.scss.

@patrickmckowen
Copy link
Contributor Author

@RachelRVasquez @fuhton

In my mind, Bootstrap was a great way to get a head start. It should not be a dependency for the long term. I anticipate only moving further from Bootstrap over time.

@fuhton
Copy link
Contributor

fuhton commented Jun 15, 2016

@pmmck That makes sense.

@RachelRVasquez While I agree, maybe it would be better to import Bootstraps sass into our styles.scss and then include our own variables.scss file afterwords.

@fuhton fuhton removed their assignment Jun 15, 2016
@patrickmckowen
Copy link
Contributor Author

@fuhton are you thinking our style.scss file would look something like (abbreviated for brevity)...

// JDRF variables and mixins
@import 'variables';
@import 'mixins';

// Bootstrap Mixins
@import 'vendor/bootstrap/mixins/border-radius';
@import 'vendor/bootstrap/mixins/clearfix';

// JDRF Mixins
@import 'mixins/buttons';

// Bootstrap CSS
@import 'normalize';
@import 'reboot';
@import 'utilities';

// JDRF Components
@import 'badges';
@import 'buttons';
@import 'button-group';
@import 'cards';
@import 'nav';

// Vendor
@import 'vendor/ripple';

cc @RachelRVasquez

@fuhton
Copy link
Contributor

fuhton commented Jun 15, 2016

More like

// Bootstrap Variables
@import 'scss/variables';

// JDRF variables and mixins
@import 'variables';
@import 'mixins';
...

@patrickmckowen
Copy link
Contributor Author

@fuhton So to understand the DS variables I have to compare two files? I have to open the Bootstrap variables file and the JDRF variables file and do a Diff?

@fuhton
Copy link
Contributor

fuhton commented Jun 15, 2016

Good point. @RachelRVasquez Thoughts?

@patrickmckowen
Copy link
Contributor Author

I'd like to only remove imports when we're overwriting Bootstrap's original file so much that it no longer makes sense to keep the import due to overwriting most of it. This is the route we took with the buttons.scss.

I agree with this approach. I think the variables file is the only exception. It's important to me that I have a single source of truth for all the variables that are controlling the DS.

@fuhton @RachelRVasquez

@fuhton
Copy link
Contributor

fuhton commented Jun 15, 2016

@pmmck Agreed. @RachelRVasquez Thoughts?

@RachelRVasquez
Copy link
Contributor

@fuhton @pmmck I agree with not splitting the variables into two separate files, especially when they're grouped together in our current file so we can see at a glance where to edit something. I'd say right now, how our current variables file is set, it's not necessary to remove the import just yet. Most of the variables from Bootstrap are still defined in Bootstrap and it appears like it's more in our file due to us adding custom variables where Bootstrap only went so far. I feel like in the future when we have more of Bootstrap's variables in our overwrite file, then it'd be time to remove the import. In the meantime, to future proof for Bootstrap updates, we'd keep it as is with the import serving as the place to look when you want to overwrite a variable we haven't already overwritten in our own file.

@patrickmckowen
Copy link
Contributor Author

@RachelRVasquez is there a variable in the Bootstrap file that is controlling anything in the Design System?

cc @fuhton

@RachelRVasquez
Copy link
Contributor

@pmmck To clarify, are you asking if we're using any variables from Bootstrap's variable file? We may not be using all of them, but we are using any that are being called from files with the @import in our overwrite files - for things like inline text elements, the grid, badges etc. Whatever we don't overwrite ourselves, it falls back to Bootstrap's imported file.

cc: @fuhton

@patrickmckowen
Copy link
Contributor Author

@RachelRVasquez I'm fine with importing variables if we aren't using them in the DS. But all variables related to the DS must be in our variables file.

cc @fuhton

@RachelRVasquez RachelRVasquez removed their assignment Jun 16, 2016
@patrickmckowen
Copy link
Contributor Author

@RachelRVasquez @fuhton let's wait until the Forms Milestone before moving forward with creating a single variables file i.e. Single Source of Truth.

@fuhton
Copy link
Contributor

fuhton commented Sep 21, 2016

@RachelRVasquez Do you have any new thoughts on this ticket? changed thoughts?

@RachelRVasquez
Copy link
Contributor

@fuhton @pmmck It's been a while since we've reviewed this so just to clarify - is our goal to ensure that any existing components from the DS have their variables in the DS stylesheet regardless of whether their values have changed from Bootstrap's defaults?

Right now our variables file is around 216 lines whereas Bootstrap's is around 638. It would seem like we've overwritten Bootstrap's variable file by a third, but I'd say about half of the DS variables are custom ones we've created and therefore don't exist in Bootstrap. I'd still like to keep the @import at this point, but we can compare with the Bootstrap file and copy any variables directly tied to our components into our file. Thoughts?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants