new-log-viewer: Override JoyUI fontFamily and radius properties globally.
#83
Conversation
WalkthroughThe changes involve updates to the theme configuration in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant JoyButton
participant JoyInput
participant JoySelect
participant JoyFormControl
User->>JoyButton: Interacts (no overrides)
User->>JoyInput: Interacts (no overrides)
User->>JoySelect: Selects option (default props)
User->>JoyFormControl: Interacts (with hover effects)
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
new-log-viewer/src/components/theme.tsx (3)
59-63: LGTM! Consider enhancing focus styles for better accessibility.The reintroduction of the
focusproperty is a good step towards centralizing theme configurations. The 3px outline width provides a clear visual indicator for focused elements.To further improve accessibility, consider adding a contrasting outline colour. For example:
focus: { default: { outlineWidth: "3px", + outlineColor: "rgba(0, 0, 0, 0.5)", + outlineStyle: "solid", }, },This change would ensure that the focus indicator is visible against various background colours.
64-66: LGTM! Consider adding fallback fonts for improved reliability.The reintroduction of the
fontFamilyproperty using a CSS variable is an excellent approach for centralized theme management.To enhance reliability, consider adding fallback fonts. For example:
fontFamily: { - body: "var(--ylv-ui-font-family)", + body: "var(--ylv-ui-font-family), -apple-system, BlinkMacSystemFont, 'Segoe UI', 'Roboto', 'Oxygen', 'Ubuntu', 'Cantarell', 'Fira Sans', 'Droid Sans', 'Helvetica Neue', sans-serif", },This change ensures that if the custom font fails to load, the system will fall back to a suitable alternative.
67-75: LGTM! Consider enhancing flexibility and removing unnecessary ESLint comments.The introduction of the
radiusproperty effectively centralizes border radius management, aligning with the PR objectives.
- To improve design flexibility, consider using different values for each size:
radius: { - /* eslint-disable sort-keys */ - xs: "2px", - sm: "2px", - md: "2px", - lg: "2px", - xl: "2px", - /* eslint-enable sort-keys */ + xs: "2px", + sm: "4px", + md: "6px", + lg: "8px", + xl: "10px", },
- Remove the ESLint disable/enable comments as they are unnecessary when the keys are already in the desired order.
These changes would provide more design options while maintaining clean, lint-free code.
|
lgtm. A minor issue is |
kirkrodrigues
changed the title
radius property in JoyUI theme config.
You're right. Could you check if e5186ab correctly does the override? |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
new-log-viewer/src/components/theme.tsx (1)
59-73: Overall improvements with suggestion for documentationThe changes successfully centralize border radius management and streamline the theme configuration, which aligns well with the PR objectives. The removal of component-specific overrides in favour of a global radius property should improve consistency across the UI.
However, to ensure these changes are well understood by the team:
Consider updating the project documentation to reflect these significant changes in theme configuration, especially:
- The new centralized radius property
- The changed focus thickness
- The removal of component-specific border radius overrides
This will help maintain clarity for future development and onboarding of new team members.
| radius: { | ||
| /* eslint-disable sort-keys */ | ||
| xs: "2px", | ||
| sm: "2px", | ||
| md: "2px", | ||
| lg: "2px", | ||
| xl: "2px", | ||
| /* eslint-enable sort-keys */ | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider varying radius sizes and review ESLint rule usage
- The radius is set to "2px" for all size variants. Consider using different radii for different sizes to provide better visual distinction between components of varying sizes. For example:
radius: {
xs: "2px",
sm: "4px",
md: "6px",
lg: "8px",
xl: "10px",
}- The ESLint
sort-keysrule is disabled for this section. If key sorting is generally preferred in the project, consider sorting the keys alphabetically to maintain consistency with other parts of the codebase.
Would you like assistance in implementing these suggestions?
Co-authored-by: Henry8192 <[email protected]>
|
For the title, how about: new-log-viewer: Apply |
How about |
junhaoliao
changed the title
radius property in JoyUI theme config.fontFamily and radius properties globally.
References
new-log-viewer series: #45 #46 #48 #51 #52 #53 #54 #55 #56 #59 #60 #61 #62 #63 #66 #67 #68 #69 #70 #71 #72 #73 #74 #76 #77 #78 #79 #80 #81 #82
Description
Validation performed
Summary by CodeRabbit
New Features
Improvements
Bug Fixes