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

[colors] feat: add modern colors in preparation for v5.x #4906

Merged
merged 16 commits into from
Sep 15, 2021

Conversation

johnly13
Copy link
Contributor

@johnly13 johnly13 commented Sep 14, 2021

Checklist

  • Includes tests
  • Update documentation

Changes proposed in this pull request:

  • Adds new colors that have more accessible contrast ratios within Blueprint components.
  • Once this is released and @blueprintjs/colors@5.x is published, I'll make follow-up PRs to:
    • Add these new colors into Blueprint core by upgrading the @blueprintjs/colors dependency and creating a new blueprint-modern-colors.scss, which contains the line @import "@blueprintjs/colors/lib/css/colors.css
    • Create new less/scss variable files in @blueprintjs/core that has the line @import "@blueprintjs/colors/lib/css/colors.css, along with the rest of the blueprint variables (will require an update to generate-css-variables.js to accept an output file name)
    • Add remaining color-related style changes to packages/colors/src/colors.scss

Reviewers should focus on:

Screenshot

Copy link
Contributor

@adidahiya adidahiya left a comment

Choose a reason for hiding this comment

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

let's add a legacyColors.ts file with the old colors...

// legacyColors.ts

export const LegacyColors = {
  // ...
};

and then core should re-export these colors to avoid a breaking change:

// core/src/common/index.ts

import { LegacyColors } from "@blueprintjs/colors";

export Colors = LegacyColors;

consumers who wish to use the new colors can just switch to the new package:

import { Colors } from "@blueprintjs/colors";

packages/colors/package.json Outdated Show resolved Hide resolved
packages/colors/package.json Outdated Show resolved Hide resolved
packages/colors/package.json Show resolved Hide resolved
packages/colors/package.json Outdated Show resolved Hide resolved
packages/core/package.json Outdated Show resolved Hide resolved
packages/stylelint-plugin/package.json Outdated Show resolved Hide resolved
@adidahiya adidahiya closed this Sep 14, 2021
@adidahiya adidahiya reopened this Sep 14, 2021
@adidahiya
Copy link
Contributor

not sure why the CI workflow wasn't getting triggered before... but after pushing my commit merging develop it looks like it's running

@blueprint-bot
Copy link

Merge remote-tracking branch 'origin/develop' into johnlee/modern-colors

Previews: documentation | landing | table

@blueprint-bot
Copy link

dist:variables in colors package

Previews: documentation | landing | table

@adidahiya
Copy link
Contributor

adidahiya commented Sep 15, 2021

I added LegacyColors. Now there are a couple remaining issues with this PR, hoping you can address them @johnly13:

Cascading variable declarations

We are moving to an API where consumers wishing to use the new colors in 3.x (and the eventual stable API in 5.0) are recommended to import multiple Sass files if they use both colors and other BP variables (like grid size):

@import "~@blueprintjs/colors/lib/scss/colors";
@import "~@blueprintjs/core/lib/scss/variables";

The problem with this is that @blueprintjs/core/lib/scss/variables.scss doesn't include the ! default syntax in its variable declarations (the generate-css-variables script strips them out), so the color values here will override the new modernized colors. And we can't import @blueprintjs/colors/lib/scss/colors.scss after variables.scss because the latter includes color aliases which depend on the color hex values. (Note: we should write a stylelint rule which enforces import ordering to avoid this error, as well)

I recommend changing generate-css-variables.js to allow retaining the ! default syntax through a CLI option so that this API with multiple Sass imports can work.

Color variables file name

The packages/colors dist:variables script produces a lib/scss/variables.scss file, when we want a lib/scss/colors.scss file.

I recommend changing generate-css-variables.js to allow specifying output filename with a --outputFilename CLI option.

@adidahiya
Copy link
Contributor

the behavior of the stylelint rule to avoid color literals has changed now. I think we should make it use LegacyColors to retain the old behavior. we can think about how to incorporate new Colors in 5.0

@blueprint-bot
Copy link

Merge remote-tracking branch 'origin' into johnlee/modern-colors

Previews: documentation | landing | table

@blueprint-bot
Copy link

Use LegacyColors instead of Colors in no-color-literal styelint rule

Previews: documentation | landing | table

@adidahiya adidahiya changed the title Add modern colors in preparation for Blueprint 5.x [colors] feat: add modern colors in preparation for Blueprint 5.x Sep 15, 2021
@adidahiya adidahiya changed the title [colors] feat: add modern colors in preparation for Blueprint 5.x [colors] feat: add modern colors in preparation for v5.x Sep 15, 2021
@adidahiya adidahiya merged commit 9977dc4 into develop Sep 15, 2021
@adidahiya adidahiya deleted the johnlee/modern-colors branch September 15, 2021 20:21
@ptgamr
Copy link

ptgamr commented Oct 22, 2021

Upgraded @blueprintjs/core yesterday and new colors look great!

However, I noticed the popover2 package is still using the old colors, is there plan to update this yet?

@adidahiya
Copy link
Contributor

@ptgamr use popover2 v1.0.0-beta.x, the new colors are there

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