feat(ArticleFeedbackv5): styling for extension#1240
feat(ArticleFeedbackv5): styling for extension#1240whostacking wants to merge 5 commits intoStarCitizenTools:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughAdds two ArticleFeedbackv5 LESS stylesheets and registers them in the Citizen skin. Updates Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Stylelint (17.3.0)skinStyles/extensions/ArticleFeedbackv5/ext.articleFeedbackv5.dashboard.styles.lessConfigurationError: Could not find "stylelint-config-wikimedia/support-modern". Do you need to install the package or use the "configBasedir" option? skinStyles/extensions/ArticleFeedbackv5/ext.articleFeedbackv5.styles.lessConfigurationError: Could not find "stylelint-config-wikimedia/support-modern". Do you need to install the package or use the "configBasedir" option? Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
skinStyles/extensions/ArticleFeedbackv5/ext.articleFeedbackv5.styles.less
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
skinStyles/extensions/ArticleFeedbackv5/ext.articleFeedbackv5.dashboard.styles.less (1)
1-25: Consider using CSS variables for transform offsets.The
translateX(-30px)on Line 23 is a magic number. Consider defining a CSS variable like--marker-icon-offsetfor better maintainability and consistency.🔎 Suggested refactor
Add a variable definition at the top of the file or in a shared variables file:
// At the top of the file or in variables @marker-icon-offset: -30px;Then use it in the transform:
- transform: translateX( -30px ) translateY( -50% ); + transform: translateX( @marker-icon-offset ) translateY( -50% );
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
skin.jsonskinStyles/extensions/ArticleFeedbackv5/ext.articleFeedbackv5.dashboard.styles.lessskinStyles/extensions/ArticleFeedbackv5/ext.articleFeedbackv5.styles.less
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: songnguxyz
Repo: StarCitizenTools/mediawiki-skins-Citizen PR: 0
File: :0-0
Timestamp: 2025-03-27T17:15:08.802Z
Learning: The MediaWiki Citizen skin is deprecating legacy CSS variables in v3.0.0, including `var(--color-base--emphasized)` which should be replaced with `var(--color-emphasized)` and `var(--border-radius--small)` which should be replaced with `var(--border-radius-base)`. These legacy variables are documented in the resources/variables.less file and marked for removal.
📚 Learning: 2025-03-27T17:15:08.802Z
Learnt from: songnguxyz
Repo: StarCitizenTools/mediawiki-skins-Citizen PR: 0
File: :0-0
Timestamp: 2025-03-27T17:15:08.802Z
Learning: The MediaWiki Citizen skin is deprecating legacy CSS variables in v3.0.0, including `var(--color-base--emphasized)` which should be replaced with `var(--color-emphasized)` and `var(--border-radius--small)` which should be replaced with `var(--border-radius-base)`. These legacy variables are documented in the resources/variables.less file and marked for removal.
Applied to files:
skin.jsonskinStyles/extensions/ArticleFeedbackv5/ext.articleFeedbackv5.dashboard.styles.less
📚 Learning: 2025-03-27T17:15:08.802Z
Learnt from: songnguxyz
Repo: StarCitizenTools/mediawiki-skins-Citizen PR: 0
File: :0-0
Timestamp: 2025-03-27T17:15:08.802Z
Learning: In the MediaWiki Citizen skin v3.0.0, several CSS variables are being deprecated, including `var(--color-base--emphasized)` which should be replaced with `var(--color-emphasized)` and `var(--border-radius--small)` which should be replaced with `var(--border-radius-base)`.
Applied to files:
skin.json
📚 Learning: 2025-03-27T17:15:05.008Z
Learnt from: songnguxyz
Repo: StarCitizenTools/mediawiki-skins-Citizen PR: 0
File: :0-0
Timestamp: 2025-03-27T17:15:05.008Z
Learning: In the mediawiki-skins-Citizen project, variable names like `var(--color-base--emphasized)` and `var(--border-radius--small)` are being deprecated in favor of `var(--color-emphasized)` and `var(--border-radius-base)` respectively in the upcoming v3.0.0 release.
Applied to files:
skin.json
🔇 Additional comments (13)
skinStyles/extensions/ArticleFeedbackv5/ext.articleFeedbackv5.dashboard.styles.less (6)
32-35: LGTM!Clean flex column layout for the feedback dashboard.
37-51: LGTM!Appropriate use of CSS variables for theming and typography.
53-60: LGTM!The use of
!importantis acceptable here as this is a skin override stylesheet that needs to override the ArticleFeedbackv5 extension's default styles.
62-80: LGTM!Well-structured feedback panel layout with proper use of flexbox and CSS variable-based theming.
82-86: LGTM!Consistent link styling using CSS variables for theming.
88-94: LGTM!The sticky positioning with calculated offset works well. The
* 2multiplier on Line 90 suggests stacking below two sticky headers, which appears intentional for the dashboard layout.skin.json (1)
539-540: LGTM!The ResourceModuleSkinStyles configuration correctly registers the two new ArticleFeedbackv5 stylesheets. The naming follows MediaWiki conventions and the placement is alphabetically correct.
skinStyles/extensions/ArticleFeedbackv5/ext.articleFeedbackv5.styles.less (6)
1-8: LGTM!The panel styling appropriately uses
!importantto override the ArticleFeedbackv5 extension's default styles. The modern oklch color function and frosted glass effect provide good visual polish.
10-31: LGTM!The button styling effectively overrides jQuery UI defaults with modern transition effects and hover states. The use of
!importantis justified for overriding the extension's styles.
33-46: LGTM!Consistent height and font sizing across button elements with appropriate use of CSS variables.
48-76: LGTM!Well-structured form styling with modern flexbox layout and consistent use of CSS variables for spacing and theming.
78-95: LGTM!Clean layout adjustments for title elements with appropriate hiding of unnecessary UI components.
97-110: LGTM!Excellent UX detail with the destructive hover state for the negative response button. Clean flexbox layout implementation.
skinStyles/extensions/ArticleFeedbackv5/ext.articleFeedbackv5.dashboard.styles.less
Outdated
Show resolved
Hide resolved
skinStyles/extensions/ArticleFeedbackv5/ext.articleFeedbackv5.styles.less
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
skinStyles/extensions/ArticleFeedbackv5/ext.articleFeedbackv5.dashboard.styles.less (1)
27-30: Replace hardcoded URL with a relative or resource loader path.Line 29 contains a hardcoded absolute URL that is non-portable across MediaWiki installations and will break if the extension version changes.
🧹 Nitpick comments (1)
skinStyles/extensions/ArticleFeedbackv5/ext.articleFeedbackv5.dashboard.styles.less (1)
53-60: Consider refactoring to avoid!importantdeclarations.Lines 54 and 58 use
!importantto override font sizes, which suggests specificity conflicts with the base extension styles. While this works, it makes the CSS harder to maintain and override in the future.If possible, consider using more specific selectors (e.g.,
.articleFeedbackv5-comment-wrap > h3) to achieve the same effect without!important.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
skin.jsonskinStyles/extensions/ArticleFeedbackv5/ext.articleFeedbackv5.dashboard.styles.lessskinStyles/extensions/ArticleFeedbackv5/ext.articleFeedbackv5.styles.less
✅ Files skipped from review due to trivial changes (1)
- skinStyles/extensions/ArticleFeedbackv5/ext.articleFeedbackv5.styles.less
🚧 Files skipped from review as they are similar to previous changes (1)
- skin.json
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-03-27T17:15:08.802Z
Learnt from: songnguxyz
Repo: StarCitizenTools/mediawiki-skins-Citizen PR: 0
File: :0-0
Timestamp: 2025-03-27T17:15:08.802Z
Learning: The MediaWiki Citizen skin is deprecating legacy CSS variables in v3.0.0, including `var(--color-base--emphasized)` which should be replaced with `var(--color-emphasized)` and `var(--border-radius--small)` which should be replaced with `var(--border-radius-base)`. These legacy variables are documented in the resources/variables.less file and marked for removal.
Applied to files:
skinStyles/extensions/ArticleFeedbackv5/ext.articleFeedbackv5.dashboard.styles.less
📚 Learning: 2025-03-27T17:15:08.802Z
Learnt from: songnguxyz
Repo: StarCitizenTools/mediawiki-skins-Citizen PR: 0
File: :0-0
Timestamp: 2025-03-27T17:15:08.802Z
Learning: In the MediaWiki Citizen skin v3.0.0, several CSS variables are being deprecated, including `var(--color-base--emphasized)` which should be replaced with `var(--color-emphasized)` and `var(--border-radius--small)` which should be replaced with `var(--border-radius-base)`.
Applied to files:
skinStyles/extensions/ArticleFeedbackv5/ext.articleFeedbackv5.dashboard.styles.less
🔇 Additional comments (7)
skinStyles/extensions/ArticleFeedbackv5/ext.articleFeedbackv5.dashboard.styles.less (7)
1-25: LGTM! Clean marker styling with consistent patterns.The marker styles use a clean pattern with shared positioning and sizing. The use of CSS custom properties for sizing and color is appropriate.
32-35: LGTM!Clean flex layout implementation for the feedback container.
37-46: LGTM! Consistent theming for sort and filter controls.The styling properly applies Citizen skin variables for consistent theming across sort and filter controls.
48-51: LGTM!Appropriate use of the subtle color variable for count wrappers.
62-80: LGTM! Appropriate layout overrides for Citizen skin theming.The feedback panel styling properly resets extension defaults and applies Citizen skin theming with appropriate spacing, colors, and border-radius values.
82-86: LGTM!Clean note link styling with appropriate use of Citizen skin variables.
88-94: No issues found. The.mixin-citizen-frosted-glassmixin is properly defined inresources/mixins.lessand is used consistently across the codebase. The code is correct.
whostacking
changed the title
|
This will probably take some time to review since I have to test for compatibility issues and find a better way to handle the icons. Apart from that, do you have screenshots or demo of the changes? |
|
feedback area: dashboard area: |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@skinStyles/extensions/ArticleFeedbackv5/ext.articleFeedbackv5.styles.less`:
- Line 1: The import statement using double quotes causes a stylelint error;
update the `@import` declaration in ext.articleFeedbackv5.styles.less (the line
"@import \"mediawiki.skin.variables.less\";") to use single quotes instead of
double quotes so it becomes `@import` 'mediawiki.skin.variables.less'; to satisfy
CI.
🧹 Nitpick comments (2)
skinStyles/extensions/ArticleFeedbackv5/ext.articleFeedbackv5.styles.less (2)
50-55: Consider using a spacing variable instead of hardcoded10px.For consistency with the rest of the file, use a CSS variable like
var(--space-sm)orvar(--space-md)instead of the hardcoded pixel value.♻️ Suggested change
.articleFeedbackv5-ui .form-row { display: flex; flex-direction: column; - gap: 10px; + gap: var( --space-sm ); margin-bottom: unset; }
99-112: Nested selector&&[ data-value='no' ]may cause specificity issues.The
&&selector concatenation creates.form-item.form-item[data-value='no'], which doubles the class specificity. While this works, a single&is typically sufficient unless you're intentionally fighting specificity wars.♻️ Suggested simplification
& .form-item { margin: unset; - &&[ data-value='no' ] { + &[ data-value='no' ] { .ui-button:hover { background: var( --color-destructive--hover ); } } }
skinStyles/extensions/ArticleFeedbackv5/ext.articleFeedbackv5.styles.less
Outdated
Show resolved
Hide resolved
Enhanced styling for the Article Feedback extension for a more consistent visual design across components. Added dedicated dashboard styles to improve presentation and usability of feedback management views.
| @import 'mediawiki.skin.variables.less'; | ||
|
|
||
| .articleFeedbackv5-panel { | ||
| width: 100% !important; |
There was a problem hiding this comment.
Are the important needed? If you need for specificity, you can either use the original selector or prefix with skin-citizen.
2 participants
Summary by CodeRabbit