-
-
Notifications
You must be signed in to change notification settings - Fork 527
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
chore: Update dark theme logos styling (Fixes #1209) #1213
Conversation
WalkthroughThe changes enhance the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Homepage
participant Feature
participant SVG
User->>Homepage: Visit homepage
Homepage->>Feature: Render features
Feature->>SVG: Check allowsDarkMode
alt Dark mode enabled
Feature->>SVG: Assign svgClassName with dark mode styling
else Light mode
Feature->>SVG: Assign svgClassName with default styling
end
SVG->>Feature: Render SVG with svgClassName
Feature->>Homepage: Display features
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
@gabrieljablonski Please review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
docs/static/img/Algolia-logo.svg
is excluded by!**/*.svg
Files selected for processing (4)
- docs/src/components/HomepageFeatures/index.tsx (1 hunks)
- docs/src/components/HomepageFeatures/styles.module.css (1 hunks)
- docs/src/components/HomepageSponsored/index.tsx (1 hunks)
- docs/src/components/HomepageSponsored/styles.module.css (1 hunks)
Files skipped from review due to trivial changes (1)
- docs/src/components/HomepageSponsored/styles.module.css
Additional comments not posted (3)
docs/src/components/HomepageFeatures/styles.module.css (1)
13-15
: LGTM!The new CSS rule correctly targets the
.githubLogo
for dark theme and ensures better contrast and visibility.docs/src/components/HomepageFeatures/index.tsx (1)
45-49
: LGTM!The changes correctly implement conditional styling for the GitHub logo based on the
Svg
prop. The logic for matching the GitHub logo and applying the class names is sound.Also applies to: 53-53
docs/src/components/HomepageSponsored/index.tsx (1)
54-58
: LGTM!The changes correctly implement conditional styling for the Algolia logo based on the
Svg
prop. The logic for matching the Algolia logo and applying the class names is sound.Also applies to: 63-63
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- docs/src/components/HomepageFeatures/index.tsx (1 hunks)
- docs/src/components/HomepageSponsored/index.tsx (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- docs/src/components/HomepageFeatures/index.tsx
- docs/src/components/HomepageSponsored/index.tsx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks mostly good, but please see the suggested changes.
They're mostly the result of running yarn run prettier docs/src
, so the code style matches our eslint rules, but I've also renamed classVar
to svgClassName
for clarity.
docs/static/img/Algolia-logo.svg
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please explain why changing the SVG was needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I changed a code inside svg which controlled the fill colour of the logo. That part was preventing the change of fill colour using CSS property: "fill: #000"
So, I removed the following part from it:
<style>.cls-1,.cls-2{fill:#000;}.cls-2{fill-rule:evenodd;}</style>
Co-authored-by: Gabriel Jablonski <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- docs/src/components/HomepageFeatures/index.tsx (1 hunks)
- docs/src/components/HomepageFeatures/styles.module.css (1 hunks)
- docs/src/components/HomepageSponsored/index.tsx (1 hunks)
- docs/src/components/HomepageSponsored/styles.module.css (1 hunks)
Files skipped from review due to trivial changes (1)
- docs/src/components/HomepageSponsored/styles.module.css
Files skipped from review as they are similar to previous changes (3)
- docs/src/components/HomepageFeatures/index.tsx
- docs/src/components/HomepageFeatures/styles.module.css
- docs/src/components/HomepageSponsored/index.tsx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please try applying this patch for an alternative instead of checking for exactly the github and algolia logos.
https://gist.github.com/gabrieljablonski/ff6371cc273b17521f27b86630ead4aa
See if you have any suggestions.
Yes, that idea is better. I will make these changes and get back to you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- docs/src/components/HomepageFeatures/index.tsx (4 hunks)
- docs/src/components/HomepageFeatures/styles.module.css (1 hunks)
- docs/src/components/HomepageSponsored/index.tsx (3 hunks)
- docs/src/components/HomepageSponsored/styles.module.css (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- docs/src/components/HomepageFeatures/index.tsx
- docs/src/components/HomepageFeatures/styles.module.css
Additional comments not posted (5)
docs/src/components/HomepageSponsored/styles.module.css (1)
33-35
: LGTM!The new CSS rule correctly improves the visibility of SVG elements in dark mode.
docs/src/components/HomepageSponsored/index.tsx (4)
17-17
: LGTM!The addition of the
allowsDarkMode
property to theFeatureItem
type is correctly typed.
34-34
: LGTM!The addition of the
allowsDarkMode
property to the Algolia entry in theFeatureList
array is consistent with the objective of improving visibility in dark mode.
55-55
: LGTM!The
Feature
function is correctly updated to include theallowsDarkMode
property in its parameters.
60-63
: LGTM!The SVG's class name is dynamically assigned using the
clsx
utility, combining the defaultfeatureSvg
class withstyles.svgDarkMode
ifallowsDarkMode
is true.
@gabrieljablonski, I have now used an alternative approach that you suggested. Please review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but please run yarn run prettier docs/src/
as there are a few code-style issues.
Also waiting on @danielbarion to give a final word on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- docs/src/components/AdsContainer/styles.css (1 hunks)
- docs/src/components/BannerSponsor/styles.css (1 hunks)
- docs/src/components/HomepageFeatures/index.tsx (4 hunks)
- docs/src/components/HomepageSponsored/index.tsx (3 hunks)
Files skipped from review due to trivial changes (2)
- docs/src/components/AdsContainer/styles.css
- docs/src/components/BannerSponsor/styles.css
Files skipped from review as they are similar to previous changes (2)
- docs/src/components/HomepageFeatures/index.tsx
- docs/src/components/HomepageSponsored/index.tsx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, thanks!
## [1.0.2](https://github.com/equinor/webviz-subsurface-components/compare/[email protected]@1.0.2) (2024-10-08) ### Bug Fixes * bump react-tooltip from 5.27.0 to 5.28.0 in /typescript ([#2300](#2300)) ([e355c99](e355c99)), closes [#1209](#1209) [ReactTooltip/react-tooltip#1213](ReactTooltip/react-tooltip#1213) [ReactTooltip/react-tooltip#1211](ReactTooltip/react-tooltip#1211) [ReactTooltip/react-tooltip#1213](ReactTooltip/react-tooltip#1213) [ReactTooltip/react-tooltip#1211](ReactTooltip/react-tooltip#1211) [ReactTooltip/react-tooltip#1203](ReactTooltip/react-tooltip#1203) [ReactTooltip/react-tooltip#1205](ReactTooltip/react-tooltip#1205) [#1209](#1209) [#1213](#1213)
## [1.5.6](https://github.com/equinor/webviz-subsurface-components/compare/[email protected]@1.5.6) (2024-10-08) ### Bug Fixes * bump react-tooltip from 5.27.0 to 5.28.0 in /typescript ([#2300](#2300)) ([e355c99](e355c99)), closes [#1209](#1209) [ReactTooltip/react-tooltip#1213](ReactTooltip/react-tooltip#1213) [ReactTooltip/react-tooltip#1211](ReactTooltip/react-tooltip#1211) [ReactTooltip/react-tooltip#1213](ReactTooltip/react-tooltip#1213) [ReactTooltip/react-tooltip#1211](ReactTooltip/react-tooltip#1211) [ReactTooltip/react-tooltip#1203](ReactTooltip/react-tooltip#1203) [ReactTooltip/react-tooltip#1205](ReactTooltip/react-tooltip#1205) [#1209](#1209) [#1213](#1213)
## [1.0.3](https://github.com/equinor/webviz-subsurface-components/compare/[email protected]@1.0.3) (2024-10-08) ### Bug Fixes * bump react-tooltip from 5.27.0 to 5.28.0 in /typescript ([#2300](#2300)) ([e355c99](e355c99)), closes [#1209](#1209) [ReactTooltip/react-tooltip#1213](ReactTooltip/react-tooltip#1213) [ReactTooltip/react-tooltip#1211](ReactTooltip/react-tooltip#1211) [ReactTooltip/react-tooltip#1213](ReactTooltip/react-tooltip#1213) [ReactTooltip/react-tooltip#1211](ReactTooltip/react-tooltip#1211) [ReactTooltip/react-tooltip#1203](ReactTooltip/react-tooltip#1203) [ReactTooltip/react-tooltip#1205](ReactTooltip/react-tooltip#1205) [#1209](#1209) [#1213](#1213)
## [1.3.11](https://github.com/equinor/webviz-subsurface-components/compare/[email protected]@1.3.11) (2024-10-08) ### Bug Fixes * bump react-tooltip from 5.27.0 to 5.28.0 in /typescript ([#2300](#2300)) ([e355c99](e355c99)), closes [#1209](#1209) [ReactTooltip/react-tooltip#1213](ReactTooltip/react-tooltip#1213) [ReactTooltip/react-tooltip#1211](ReactTooltip/react-tooltip#1211) [ReactTooltip/react-tooltip#1213](ReactTooltip/react-tooltip#1213) [ReactTooltip/react-tooltip#1211](ReactTooltip/react-tooltip#1211) [ReactTooltip/react-tooltip#1203](ReactTooltip/react-tooltip#1203) [ReactTooltip/react-tooltip#1205](ReactTooltip/react-tooltip#1205) [#1209](#1209) [#1213](#1213)
## [2.1.1](https://github.com/equinor/webviz-subsurface-components/compare/[email protected]@2.1.1) (2024-10-08) ### Bug Fixes * bump react-tooltip from 5.27.0 to 5.28.0 in /typescript ([#2300](#2300)) ([e355c99](e355c99)), closes [#1209](#1209) [ReactTooltip/react-tooltip#1213](ReactTooltip/react-tooltip#1213) [ReactTooltip/react-tooltip#1211](ReactTooltip/react-tooltip#1211) [ReactTooltip/react-tooltip#1213](ReactTooltip/react-tooltip#1213) [ReactTooltip/react-tooltip#1211](ReactTooltip/react-tooltip#1211) [ReactTooltip/react-tooltip#1203](ReactTooltip/react-tooltip#1203) [ReactTooltip/react-tooltip#1205](ReactTooltip/react-tooltip#1205) [#1209](#1209) [#1213](#1213)
This PR fixes the Issue #1209. Now, when you toggle dark mode on the page, the GitHub and Algolia logos adapt to the change in background colour:
Demo Video: https://streamable.com/79bbls
Before:
After:
Before:
After:
Summary by CodeRabbit
New Features
Style