Skip to content

docs(stepper): Make CDK stepper example accessible in dark mode. #20013

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Jul 22, 2020
Merged

docs(stepper): Make CDK stepper example accessible in dark mode. #20013

merged 4 commits into from
Jul 22, 2020

Conversation

walvekarnikhil
Copy link
Contributor

Removed hard-coded colors and replaced with inherit colors.
Used font-weight to show active step.

Fixes #17152

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jul 16, 2020
@walvekarnikhil
Copy link
Contributor Author

walvekarnikhil commented Jul 17, 2020 •
edited
Loading

Do we have flaky tests?
Same tests failed for #19985

@walvekarnikhil
Copy link
Contributor Author

Here are the screenshots for the CDK stepper examples.
Light mode:
Screenshot 2020-07-15 at 10 05 32 PM

Dark mode:
Screenshot 2020-07-15 at 10 03 08 PM

@walvekarnikhil
Copy link
Contributor Author

It seems we have a flaky test, it passed after a fake commit.

crisbeto
crisbeto reviewed Jul 21, 2020
View reviewed changes
Copy link
Member

@crisbeto crisbeto left a comment

Choose a reason for hiding this comment

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

A couple more notes:

  • The commit message should be prefixed with docs(stepper) instead of stepper(guide).
  • If you rebase against the latest master the CI flakes that you saw earlier should be gone.

src/components-examples/cdk/stepper/cdk-custom-stepper-without-form/example-custom-stepper.css Outdated
padding: 10px;
margin: 10px;
color: inherit;
Copy link
Member

Choose a reason for hiding this comment

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

I think all of these color: inherit uses are redundant. Doesn't color default to inherit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, kept color: inherit for nav buttons since those were shown in black for dark mode.

src/components-examples/cdk/stepper/cdk-custom-stepper-without-form/example-custom-stepper.css
}

.example-step.example-active {
color: blue;
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can pick a color that stands out both on light and dark backgrounds? I believe the point of this example is to make it stand out to illustrate a point, rather than be pretty.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes right, we discussed that point but there is an issue with maintaining text contrast for both light and dark mode. So instead of using a neutral color, we ended using the text colors.

Copy link
Member

Choose a reason for hiding this comment

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

Alright, the font-weight that you replaced it with should be fine as well.

@walvekarnikhil walvekarnikhil changed the title stepper(guide): Make example accessible in dark mode. docs(stepper): Make example accessible in dark mode. Jul 22, 2020
@walvekarnikhil walvekarnikhil changed the title docs(stepper): Make example accessible in dark mode. stepper(guide): Make CDK stepper example accessible in dark mode. Jul 22, 2020
@walvekarnikhil walvekarnikhil force-pushed the stepper-guide-example-dark-mode branch from 9ac10e4 to 33f6271 Compare July 22, 2020 02:42
crisbeto
crisbeto approved these changes Jul 22, 2020
View reviewed changes
Copy link
Member

@crisbeto crisbeto left a comment

Choose a reason for hiding this comment

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

LGTM, but the commit message still needs to be fixed up to say docs(stepper).

@crisbeto crisbeto added merge: fix commit message When the PR is merged, rewrites/fixups of the commit messages are needed docs This issue is related to documentation lgtm action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release labels Jul 22, 2020
@walvekarnikhil
docs(stepper): Make CDK stepper example accessible in dark mode.
093a172 
Removed hard-coded colors and replaced with inherit colors.
Used font-weight to show active step.

Fixes angular#17152
@walvekarnikhil
Changing order of styles.
9255682
@walvekarnikhil
Triggering CircleCI build again.
46da02d
@walvekarnikhil
Removing color inherit, since it is not required.
2aeac3e 
It is still required for nav buttons, since those show in the black.
@walvekarnikhil walvekarnikhil force-pushed the stepper-guide-example-dark-mode branch from 33f6271 to 2aeac3e Compare July 22, 2020 06:56
@walvekarnikhil
Copy link
Contributor Author

@crisbeto , fixed the commit message, earlier I had just modified PR title my bad.
Do I need to squash other commits also or those are done when we merge the PR?

@crisbeto
Copy link
Member

I think you still need to change your PR title, because that's what will be used when the caretaker squashes the commits.

@walvekarnikhil walvekarnikhil changed the title stepper(guide): Make CDK stepper example accessible in dark mode. docs(stepper): Make CDK stepper example accessible in dark mode. Jul 22, 2020
@walvekarnikhil
Copy link
Contributor Author

I think you still need to change your PR title, because that's what will be used when the caretaker squashes the commits.

Title updated.

@wagnermaciel wagnermaciel merged commit 5f76afa into angular:master Jul 22, 2020
wagnermaciel pushed a commit that referenced this pull request Jul 22, 2020
@walvekarnikhil @wagnermaciel
docs(stepper): Make CDK stepper example accessible in dark mode. (#20013
ae75fab 
)

* docs(stepper): Make CDK stepper example accessible in dark mode.
Removed hard-coded colors and replaced with inherit colors.
Used font-weight to show active step.

Fixes #17152

* Changing order of styles.

* Triggering CircleCI build again.

* Removing color inherit, since it is not required.
It is still required for nav buttons, since those show in the black.

(cherry picked from commit 5f76afa)
Splaktar
Splaktar reviewed Jul 22, 2020
View reviewed changes
Copy link
Contributor

@Splaktar Splaktar left a comment

Choose a reason for hiding this comment

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

Thank you for making these changes!

@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Aug 22, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Splaktar Splaktar Splaktar left review comments

@crisbeto crisbeto crisbeto approved these changes

@andrewseguin andrewseguin Awaiting requested review from andrewseguin

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement docs This issue is related to documentation merge: fix commit message When the PR is merged, rewrites/fixups of the commit messages are needed target: patch This PR is targeted for the next patch release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

stepper(guide): example inaccessible in dark mode
5 participants
@walvekarnikhil @crisbeto @Splaktar @googlebot @wagnermaciel