Repository navigation
chore(#158): allow classes to pass into link.vue if provided - #159
Conversation
WalkthroughAdds a boolean prop Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (1)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Link.vue(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build (16.x)
🔇 Additional comments (2)
src/components/Link.vue (2)
40-43: LGTM: Prop definition is correct.The
classesprop is properly defined with appropriate type and default value.
4-4: LGTM: Template binding correctly updated.The template correctly binds to the new
computedClassescomputed property, assuming the logic withincomputedClassesis fixed per the previous comment.
| computedClasses: ({ classes, disabled, isExternal, themeClass }) => { | ||
| const result = { | ||
| [themeClass]: true | ||
| }; | ||
|
|
||
| if (classes) { | ||
| // Parse the classes string and add each class to the result | ||
| classes.split(" ").forEach(className => { | ||
| if (className.trim()) { | ||
| result[className.trim()] = true; | ||
| } | ||
| }); | ||
| } else { | ||
| // Add default classes only if no classes provided | ||
| result["ct-link"] = true; | ||
| result["ct-link--disabled"] = disabled; | ||
| result["ct-link--external"] = isExternal; | ||
| } | ||
|
|
||
| return result; | ||
| }, |
There was a problem hiding this comment.
Critical: Custom classes replace defaults instead of merging.
The current logic completely omits default classes (ct-link, ct-link--disabled, ct-link--external) when custom classes are provided. This causes:
- Loss of base
ct-linkstyling - Disabled and external states are no longer reflected in CSS classes
- Component breaks visually and functionally
Additionally, an empty string "" is truthy in JavaScript, so passing classes="" would result in only themeClass being applied, losing all default classes.
Apply this diff to merge custom classes with defaults instead of replacing them:
- computedClasses: ({ classes, disabled, isExternal, themeClass }) => {
- const result = {
- [themeClass]: true
- };
-
- if (classes) {
- // Parse the classes string and add each class to the result
- classes.split(" ").forEach(className => {
- if (className.trim()) {
- result[className.trim()] = true;
- }
- });
- } else {
- // Add default classes only if no classes provided
- result["ct-link"] = true;
- result["ct-link--disabled"] = disabled;
- result["ct-link--external"] = isExternal;
- }
-
- return result;
- },
+ computedClasses: ({ classes, disabled, isExternal, themeClass }) => {
+ const result = {
+ "ct-link": true,
+ "ct-link--disabled": disabled,
+ "ct-link--external": isExternal,
+ [themeClass]: true
+ };
+
+ // Merge custom classes if provided
+ if (classes && classes.trim()) {
+ classes.split(" ").forEach(className => {
+ if (className.trim()) {
+ result[className.trim()] = true;
+ }
+ });
+ }
+
+ return result;
+ },📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| computedClasses: ({ classes, disabled, isExternal, themeClass }) => { | |
| const result = { | |
| [themeClass]: true | |
| }; | |
| if (classes) { | |
| // Parse the classes string and add each class to the result | |
| classes.split(" ").forEach(className => { | |
| if (className.trim()) { | |
| result[className.trim()] = true; | |
| } | |
| }); | |
| } else { | |
| // Add default classes only if no classes provided | |
| result["ct-link"] = true; | |
| result["ct-link--disabled"] = disabled; | |
| result["ct-link--external"] = isExternal; | |
| } | |
| return result; | |
| }, | |
| computedClasses: ({ classes, disabled, isExternal, themeClass }) => { | |
| const result = { | |
| "ct-link": true, | |
| "ct-link--disabled": disabled, | |
| "ct-link--external": isExternal, | |
| [themeClass]: true | |
| }; | |
| // Merge custom classes if provided | |
| if (classes && classes.trim()) { | |
| classes.split(" ").forEach(className => { | |
| if (className.trim()) { | |
| result[className.trim()] = true; | |
| } | |
| }); | |
| } | |
| return result; | |
| }, |
🤖 Prompt for AI Agents
In src/components/Link.vue around lines 83 to 103, the computedClasses function
currently replaces default classes when a classes string is provided and treats
an empty string as truthy; change it so default classes (ct-link,
ct-link--disabled, ct-link--external) are always set on the result, then parse
the classes string (guarding against empty/whitespace) and add each custom class
into the result; ensure disabled and isExternal still control their respective
class flags and that trimming/splitting handles multiple spaces without removing
defaults so custom classes are merged with, not replace, the defaults.
0aedba3 to
9b5d5bd
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Link.vue(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build (16.x)
| classes: ({ disabled, isExternal, themeClass, noClasses }) => ({ | ||
| 'ct-link': !noClasses, | ||
| 'ct-link--disabled': disabled && !noClasses, | ||
| 'ct-link--external': isExternal && !noClasses, | ||
| [themeClass]: !noClasses | ||
| }), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/components/Link.vue | head -150Repository: Realityloop/civictheme-vue
Length of output: 2924
🏁 Script executed:
rg -n --type=vue --type=js --type=ts 'noClasses' -g '!src/components/Link.vue'Repository: Realityloop/civictheme-vue
Length of output: 97
🏁 Script executed:
rg -n --type=vue --type=js --type=ts 'ct-link--disabled|ct-link--external'Repository: Realityloop/civictheme-vue
Length of output: 97
🏁 Script executed:
rg 'noClasses' --type=js -iRepository: Realityloop/civictheme-vue
Length of output: 463
🏁 Script executed:
cat -n src/mixins/theme.js | head -50Repository: Realityloop/civictheme-vue
Length of output: 320
🏁 Script executed:
rg 'ct-link' --type=js --type=css -A 2 -B 2Repository: Realityloop/civictheme-vue
Length of output: 5521
🏁 Script executed:
fd -e css -e scss | xargs cat | grep -A 5 'ct-link--disabled\|ct-link--external' | head -50Repository: Realityloop/civictheme-vue
Length of output: 52
🏁 Script executed:
find . -name '*.css' -o -name '*.scss' | xargs grep -l 'ct-link'Repository: Realityloop/civictheme-vue
Length of output: 54
🏁 Script executed:
git log --oneline -20 -- src/components/Link.vueRepository: Realityloop/civictheme-vue
Length of output: 138
🏁 Script executed:
git show 9b5d5bd --statRepository: Realityloop/civictheme-vue
Length of output: 50382
🏁 Script executed:
git show 9b5d5bdRepository: Realityloop/civictheme-vue
Length of output: 50384
🏁 Script executed:
git show 9b5d5bd -- src/components/Link.vueRepository: Realityloop/civictheme-vue
Length of output: 2644
Don't remove theme and accessibility classes when noClasses is true.
When noClasses is true, the current implementation removes themeClass and the modifier classes (ct-link--disabled, ct-link--external). This is problematic:
-
Theme removal: The
themeClass(e.g.,ct-theme-light) applies essential theming that controls link appearance across the application. Removing it breaks the design system. -
Accessibility: The modifier classes provide important visual state feedback for users. Removing
ct-link--disabledandct-link--externaleliminates these indicators.
If the intent of noClasses is to remove only the base styling while preserving semantic structure, the computed property should keep the theme and state classes:
classes: ({ disabled, isExternal, themeClass, noClasses }) => ({
'ct-link': !noClasses,
'ct-link--disabled': disabled,
'ct-link--external': isExternal,
[themeClass]: true
})This way noClasses still removes the base .ct-link class but preserves theming and accessibility markers.
🤖 Prompt for AI Agents
In src/components/Link.vue around lines 83 to 88, the classes computed property
currently drops themeClass and modifier classes when noClasses is true; update
the logic so noClasses only omits the base 'ct-link' class while always
including the themeClass and still adding state modifiers when applicable (i.e.,
keep 'ct-link--disabled' when disabled, 'ct-link--external' when isExternal, and
always include themeClass regardless of noClasses).
9b5d5bd to
06abef0
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Link.vue(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build (16.x)
🔇 Additional comments (1)
src/components/Link.vue (1)
4-4: Good: Enables external class merging via Vue's standard mechanism.The template binding change from
:class="classes"to:class="[classes, $attrs.class]"successfully achieves the PR objective. Using$attrs.classis the Vue-idiomatic way to allow external classes to be passed in and merged, rather than creating a customclassesprop. When consumers use<Link class="my-custom-class">, those classes will now be merged with the component's internal classes.
| noClasses: { | ||
| type: Boolean, | ||
| default: false | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Suggest renaming noClasses for clarity.
The prop name noClasses is misleading because it doesn't remove all classes—it only omits the base ct-link class while preserving modifiers (ct-link--disabled, ct-link--external) and the theme class. Consider renaming to noBaseClass or excludeBaseClass to more accurately reflect its behavior.
Additionally, the use case for this prop isn't documented. Please add a comment or documentation explaining when developers should use this flag (e.g., when they want to provide completely custom styling while maintaining semantic state classes).
Apply this diff:
- noClasses: {
+ noBaseClass: {
type: Boolean,
default: false
},And update the computed property accordingly:
- classes: ({ disabled, isExternal, themeClass, noClasses }) => {
+ classes: ({ disabled, isExternal, themeClass, noBaseClass }) => {
const cls = {}
- if (!noClasses) cls['ct-link'] = true
+ if (!noBaseClass) cls['ct-link'] = true
if (disabled) cls['ct-link--disabled'] = true
if (isExternal) cls['ct-link--external'] = true
if (themeClass) cls[themeClass] = true
return cls
},Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/components/Link.vue around lines 64 to 67, rename the prop noClasses to
noBaseClass (or excludeBaseClass) to accurately reflect that it only omits the
base "ct-link" class while keeping modifier and theme classes; update the prop
declaration name and default, update any usages and the computed class-building
property to check the new prop name, and add a brief inline comment above the
prop explaining the intended use (e.g., use when you want to omit only the base
ct-link class but retain semantic modifier/theme classes for state and
accessibility).
| classes: ({ disabled, isExternal, themeClass, noClasses }) => { | ||
| const cls = {} | ||
| if (!noClasses) cls['ct-link'] = true | ||
| if (disabled) cls['ct-link--disabled'] = true | ||
| if (isExternal) cls['ct-link--external'] = true | ||
| if (themeClass) cls[themeClass] = true | ||
| return cls | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "*.vue" -o -name "*.js" | grep -i "link\|theme" | head -20Repository: Realityloop/civictheme-vue
Length of output: 244
🏁 Script executed:
cat -n src/components/Link.vue | head -100Repository: Realityloop/civictheme-vue
Length of output: 2822
🏁 Script executed:
fd -type f -name "*.js" | xargs rg -l "ThemeMixin\|themeClass" | head -10Repository: Realityloop/civictheme-vue
Length of output: 240
🏁 Script executed:
cat -n src/mixins/theme.jsRepository: Realityloop/civictheme-vue
Length of output: 320
🏁 Script executed:
rg -n "themeClass" src/mixins/theme.js -A 5 -B 5Repository: Realityloop/civictheme-vue
Length of output: 194
Remove redundant guard on themeClass.
The if (themeClass) check on line 88 is unnecessary. The ThemeMixin defines themeClass as a computed property that always returns a string: `ct-theme-${theme}`, where theme defaults to 'light'. This means themeClass is always a non-empty string and will never be falsy. Remove the conditional guard and directly assign the theme class:
cls[themeClass] = true🤖 Prompt for AI Agents
In src/components/Link.vue around lines 83 to 90, the guard checking `if
(themeClass)` is redundant because ThemeMixin guarantees `themeClass` is always
a non-empty string; remove that conditional and always set `cls[themeClass] =
true` so the theme class is always applied, leaving the other guards intact.
06abef0 to
eacdbe6
Compare
eacdbe6 to
64ff34c
Compare
Types of changes
Description
Checklist:
Screenshots/Media:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.