Skip to content

[APT-1628] Fix icons for theme toggle switch #120

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 2 commits into from
Jan 5, 2022
Merged

[APT-1628] Fix icons for theme toggle switch #120

merged 2 commits into from
Jan 5, 2022

Conversation

ebeneliason
Copy link
Contributor

These were applied with styles that should have been associated with the
module. We now swizzle the Toggle component to adjust the behavior.

@netlify
Copy link

netlify bot commented Jan 3, 2022 •
edited
Loading

✔️ Deploy Preview for pensive-meitner-faaeee ready!

🔨 Explore the source changes: 9b286cb

🔍 Inspect the deploy log: https://app.netlify.com/sites/pensive-meitner-faaeee/deploys/61d5cd185fbc4100076567a6

😎 Browse the preview: https://deploy-preview-120--pensive-meitner-faaeee.netlify.app

oredavids
oredavids previously approved these changes Jan 4, 2022
View reviewed changes
Copy link
Contributor

@oredavids oredavids left a comment

Choose a reason for hiding this comment

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

LGTM.

@oredavids oredavids mentioned this pull request Jan 4, 2022
Merged
@ebeneliason ebeneliason changed the title [APT-1628] Fix icons for theme toggle switch [WIP] [APT-1628] Fix icons for theme toggle switch Jan 4, 2022
@ebeneliason
Copy link
Contributor Author

We need to wait to re-swizzle this once #123 is merged.

@oredavids
Copy link
Contributor

#123 now merged.

@ebeneliason
Fix icons for theme toggle switch
56e75cd 
These were applied with styles that should have been associated with the
module. We now swizzle the Toggle component to adjust the behavior.
@ebeneliason ebeneliason changed the title [WIP] [APT-1628] Fix icons for theme toggle switch [APT-1628] Fix icons for theme toggle switch Jan 5, 2022
@ebeneliason
Copy link
Contributor Author

ebeneliason commented Jan 5, 2022 •
edited
Loading

We need to wait to re-swizzle this once #123 is merged.

I tried rebasing locally on the latest master which includes the Docusaurus upgrade, and re-swizzling the Toggle component. The resulting diff, after adding back in the custom CSS we added, is empty. As such, I think there's nothing else to do here and we can merge this as is. @oredavids Any hesitation?

@oredavids
Copy link
Contributor

@ebeneliason that's curious but I don't have any hesitation given the supposed improvements were meant to be a polish. Perhaps it was in a dependency.

@ebeneliason
Copy link
Contributor Author

Oops — I just failed to run yarn to update locally before swizzling. I'll push a commit with the change in a moment.

@ebeneliason ebeneliason force-pushed the apt-1628-theme-toggle-icons branch from 94fed6f to 9b286cb Compare January 5, 2022 16:53
@ebeneliason
Copy link
Contributor Author

Ready for re-review.

oredavids
oredavids approved these changes Jan 5, 2022
View reviewed changes
Copy link
Contributor

@oredavids oredavids left a comment

Choose a reason for hiding this comment

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

LGTM.

@ebeneliason ebeneliason merged commit 5007a5f into master Jan 5, 2022
@ebeneliason ebeneliason deleted the apt-1628-theme-toggle-icons branch January 5, 2022 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@oredavids oredavids oredavids approved these changes

@eak12913 eak12913 Awaiting requested review from eak12913

@pete0emerson pete0emerson Awaiting requested review from pete0emerson

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ebeneliason @oredavids