fix(home): hero vertical centering, overlap prop, and Vuetify 4 typography migration#3588
Open
PierreBrisorgueil wants to merge 3 commits intomasterfrom
Open
fix(home): hero vertical centering, overlap prop, and Vuetify 4 typography migration#3588PierreBrisorgueil wants to merge 3 commits intomasterfrom
PierreBrisorgueil wants to merge 3 commits intomasterfrom
Conversation
📝 WalkthroughWalkthroughAdds an Changes
Sequence Diagram(s)sequenceDiagram
participant Component as VMarkdown Component
participant Marked as marked (parse / parseInline)
participant DOMPurify as DOMPurify.sanitize
participant Renderer as Vue Renderer (VNode)
Component->>Marked: call parseInline(source) if inline=true\notherwise parse(source)
Marked-->>Component: HTML string or throws
alt parse success
Component->>DOMPurify: sanitize(HTML)
DOMPurify-->>Component: safeHTML
Component->>Renderer: create VNode (<span> or <div>) with innerHTML=safeHTML
Renderer-->>Component: rendered VNode
else parse error
Marked-->>Component: throws error
Component->>Renderer: create fallback VNode (<span> or <div>) with raw text
Renderer-->>Component: rendered fallback
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3588 +/- ##
=======================================
Coverage 95.93% 95.93%
=======================================
Files 20 20
Lines 516 517 +1
Branches 140 143 +3
=======================================
+ Hits 495 496 +1
Misses 21 21 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This pull request comprehensively migrates the codebase from Vuetify 3 to Vuetify 4 Material Design 3 typography naming conventions, while simultaneously fixing critical layout issues in the home module's hero and statistics components.
Changes:
- Migrated all Vuetify 3 typography classes to Vuetify 4 MD3 naming across 11 files (text-h1→text-display-*, text-body-1→text-body-large, etc.)
- Fixed hero and statistics vertical centering by adding
fill-heightclass to v-row and implementing proper containerStyle with 65px top padding - Added
overlapprop to hero component supporting Boolean, String, and Object types with responsive defaults - Migrated VMarkdown plugin from rendering
<span>to<div>to fix invalid HTML structure, updated component usage withd-inlineclass, and updated test expectations
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/modules/home/views/home.view.vue | Passes overlap config prop to hero component |
| src/modules/home/components/home.hero.component.vue | Implements overlap prop, containerStyle computed property with proper padding, adds fill-height to v-row, migrates typography classes, and refactors VMarkdown usage with d-inline |
| src/modules/home/components/home.statistics.component.vue | Adds fill-height to v-row for both blur and parallax variants, migrates typography classes |
| src/modules/home/components/home.gallery.component.vue | Migrates typography classes for tabs and carousel content |
| src/modules/home/components/home.steps.component.vue | Migrates typography classes for step titles |
| src/modules/home/components/home.social.component.vue | Migrates typography classes for social section title and names |
| src/modules/home/components/utils/home.img.component.vue | Migrates typography classes for card title and text |
| src/modules/home/components/utils/home.dynamicIsland.component.vue | Migrates typography class for dynamic island text |
| src/modules/home/components/utils/home.content.component.vue | Migrates typography classes for title, subtitle, text, and button |
| src/modules/core/components/core.header.component.vue | Migrates typography classes for navigation links, sublinks, and mobile menu |
| src/modules/core/components/core.footer.component.vue | Migrates typography classes for footer section titles and labels |
| src/modules/app/app.vue | Updates global CSS selectors from Vuetify 3 to Vuetify 4 MD3 typography classes |
| src/lib/plugins/markdown.js | Changes VMarkdown render element from span to div |
| src/lib/plugins/tests/markdown.spec.js | Updates test expectations from span to div |
| src/config/defaults/development.js | Adds overlap: true to hero config |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/lib/plugins/tests/markdown.spec.js (1)
69-77:⚠️ Potential issue | 🟡 MinorUpdate stale test description to match new wrapper
The test name still says “raw source span” while Line 77 now asserts
'div'. Rename it to avoid confusion during failures.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/plugins/tests/markdown.spec.js` around lines 69 - 77, Rename the test title to reflect the new wrapper element: update the it(...) description that currently reads "render() falls back to raw source span when marked.parse throws" to something like "render() falls back to raw source div when marked.parse throws" (or "raw source wrapper div") so it matches the assertion against vnode.type === 'div'; the change should be made alongside the existing test using getComponent() and its render call to keep intent and behavior unchanged.src/lib/plugins/markdown.js (1)
27-35:⚠️ Potential issue | 🟡 MinorAdd JSDoc for modified
render()
render()was modified but has no JSDoc with one-line description and@returns.As per coding guidelines, "/*.{js,ts,vue}: Every new or modified function must have a JSDoc header with one-line description,
@paramfor each argument, and@returnsfor any non-void return value (always include@returnsfor async functions)".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/plugins/markdown.js` around lines 27 - 35, Add a JSDoc header for the modified render() method describing its purpose in one line, document the this.source parameter (or clarify there are no formal args) with `@param` if applicable, and include an `@returns` explaining the returned VNode or null/HTMLElement string fallback; apply this to the render() function that calls marked.parse(this.source) and returns h('div', { innerHTML: html }) or h('div', this.source) in the catch block so the signature and return type are clearly documented.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/plugins/markdown.js`:
- Around line 30-31: The render() function currently lacks JSDoc and assigns
unsanitized HTML from marked.parse(this.source) directly to innerHTML, creating
an XSS risk; add a JSDoc block to the render method including `@returns`, and
sanitize the parsed HTML (e.g., pass marked.parse(this.source) through
DOMPurify.sanitize or an equivalent sanitizer) before returning h('div', {
innerHTML: sanitizedHtml }); ensure you import and use the sanitizer in the
module so user-controlled this.source cannot inject unsafe markup.
In `@src/modules/home/components/home.hero.component.vue`:
- Around line 185-196: Add a JSDoc header above the containerStyle() method
describing its purpose and include a `@returns` tag that documents the returned
style object; since it takes no explicit parameters, omit `@param` or optionally
document that it reads component props/state (this.overlap) if you follow
project conventions, and ensure the one-line description plus the `@returns`
detailing that it returns an object with 'padding-top' and computed
'padding-bottom' values for responsive layout.
- Around line 64-68: VMarkdown is a block-level component (its render returns
h('div', ...)), so embedding it directly inside heading/paragraph elements (the
VMarkdown usages in HomeHero component) breaks HTML semantics; either (A) wrap
the heading/subtitle content in a block-level container (replace the <h2> / <p>
wrappers with <div> or keep semantic heading but move VMarkdown out into a
sibling <div> and apply the same classes/ARIA roles), or (B) add an inline mode
to VMarkdown (update the markdown plugin render function to accept a prop like
inline that uses marked.parseInline() and returns a <span> instead of <div>),
then switch the HomeHero VMarkdown usages to inline mode; reference the
VMarkdown component and the markdown render function to implement and wire the
inline flag or adjust the markup accordingly.
---
Outside diff comments:
In `@src/lib/plugins/markdown.js`:
- Around line 27-35: Add a JSDoc header for the modified render() method
describing its purpose in one line, document the this.source parameter (or
clarify there are no formal args) with `@param` if applicable, and include an
`@returns` explaining the returned VNode or null/HTMLElement string fallback;
apply this to the render() function that calls marked.parse(this.source) and
returns h('div', { innerHTML: html }) or h('div', this.source) in the catch
block so the signature and return type are clearly documented.
In `@src/lib/plugins/tests/markdown.spec.js`:
- Around line 69-77: Rename the test title to reflect the new wrapper element:
update the it(...) description that currently reads "render() falls back to raw
source span when marked.parse throws" to something like "render() falls back to
raw source div when marked.parse throws" (or "raw source wrapper div") so it
matches the assertion against vnode.type === 'div'; the change should be made
alongside the existing test using getComponent() and its render call to keep
intent and behavior unchanged.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
src/config/defaults/development.jssrc/lib/plugins/markdown.jssrc/lib/plugins/tests/markdown.spec.jssrc/modules/app/app.vuesrc/modules/core/components/core.footer.component.vuesrc/modules/core/components/core.header.component.vuesrc/modules/home/components/home.gallery.component.vuesrc/modules/home/components/home.hero.component.vuesrc/modules/home/components/home.social.component.vuesrc/modules/home/components/home.statistics.component.vuesrc/modules/home/components/home.steps.component.vuesrc/modules/home/components/utils/home.content.component.vuesrc/modules/home/components/utils/home.dynamicIsland.component.vuesrc/modules/home/components/utils/home.img.component.vuesrc/modules/home/views/home.view.vue
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/plugins/markdown.js (1)
16-20:⚠️ Potential issue | 🟡 MinorAdd required
@paramand@returnstags toinstallJSDoc.The modified
installfunction at Line [20] still has an incomplete JSDoc block (no@param/@returns).Suggested fix
/** * Plugin setup. + * `@param` {import('vue').App} app - Vue application instance. + * `@returns` {void} */ export default { install: (app) => {As per coding guidelines: "
**/*.{js,ts,vue}: Every new or modified function must have a JSDoc header with one-line description,@paramfor each argument, and@returnsfor any non-void return value (always include@returnsfor async functions)`."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/plugins/markdown.js` around lines 16 - 20, The JSDoc for the exported plugin's install function is missing `@param` and `@returns` tags; update the JSDoc for the install method on the default export (install: (app) => { ... }) to include a one-line description, an `@param` describing the `app` parameter (type and role, e.g., app: Vue application instance), and an `@returns` tag (void) — include `@returns` even if the function is synchronous so it complies with the project's rule for async/non-void functions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/plugins/tests/markdown.spec.js`:
- Line 4: Add a JSDoc header to every newly added or modified function in this
test file—specifically add a one-line description plus `@param` for each parameter
and `@returns` for any non-void or async return—starting with the mock factory
parseInlineMock and extending to all mock callbacks and test callback functions
in this file; update each function/mocked callback definition (e.g.,
parseInlineMock and the other mock/test functions) to include the one-line
description, `@param` tags for each argument, and `@returns` describing the return
value (or void) per the repository guideline.
---
Outside diff comments:
In `@src/lib/plugins/markdown.js`:
- Around line 16-20: The JSDoc for the exported plugin's install function is
missing `@param` and `@returns` tags; update the JSDoc for the install method on the
default export (install: (app) => { ... }) to include a one-line description, an
`@param` describing the `app` parameter (type and role, e.g., app: Vue application
instance), and an `@returns` tag (void) — include `@returns` even if the function is
synchronous so it complies with the project's rule for async/non-void functions.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
package.jsonsrc/lib/plugins/markdown.jssrc/lib/plugins/tests/markdown.spec.jssrc/modules/home/components/home.hero.component.vue
🚧 Files skipped from review as they are similar to previous changes (1)
- src/modules/home/components/home.hero.component.vue
Collaborator
Author
|
Fixed in 2f8e212 — added JSDoc headers to |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/lib/plugins/tests/markdown.spec.js (1)
35-39:⚠️ Potential issue | 🟡 MinorMissing required JSDoc on modified callbacks/functions.
Several newly modified function literals in this file (
beforeEachand added/updateditcallbacks) still do not have JSDoc headers. Please add a one-line description plus@param/@returnswhere applicable for each changed function.As per coding guidelines: "
**/*.{js,ts,vue}: Every new or modified function must have a JSDoc header with one-line description,@paramfor each argument, and@returnsfor any non-void return value (always include@returnsfor async functions)`."Also applies to: 57-60, 72-79, 81-89, 90-100, 102-126
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/plugins/tests/markdown.spec.js` around lines 35 - 39, Add JSDoc headers to each modified function literal: the beforeEach callback and every anonymous test callback passed to it (the various it(...) blocks around the parseMock.mockImplementation / parseInlineMock.mockImplementation usage). For each callback add a one-line description, list `@param` entries for any parameters the callback receives (or state none if it accepts no args), and include `@returns` for any function that returns a value or is async (always include `@returns` for async callbacks). Ensure the comments are placed immediately above the function literal passed to beforeEach and each it so they cover the exact callback being modified.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/lib/plugins/tests/markdown.spec.js`:
- Around line 35-39: Add JSDoc headers to each modified function literal: the
beforeEach callback and every anonymous test callback passed to it (the various
it(...) blocks around the parseMock.mockImplementation /
parseInlineMock.mockImplementation usage). For each callback add a one-line
description, list `@param` entries for any parameters the callback receives (or
state none if it accepts no args), and include `@returns` for any function that
returns a value or is async (always include `@returns` for async callbacks).
Ensure the comments are placed immediately above the function literal passed to
beforeEach and each it so they cover the exact callback being modified.
…raphy migration - Fix v-row missing fill-height in hero and statistics causing align=center no-op - Fix hero content offset: padding-top 65px on v-container to account for navbar overlap - Add overlap prop to hero component (mirrors overlapStyle pattern from other home sections) - Migrate VMarkdown plugin from span to div to fix invalid HTML block-in-inline nesting - Move Vuetify typography classes to native elements above VMarkdown (d-inline pattern) - Migrate all Vuetify 3 typography classes to Vuetify 4 MD3 across 11 files: text-h1-h6 -> text-display/headline-*, text-body-1/2 -> text-body-large/medium, text-subtitle-1/2 -> text-title-large/medium, text-caption -> text-label-small Closes #3587
- Add DOMPurify sanitization to VMarkdown to prevent XSS (marked does not sanitize by default) - Add inline prop to VMarkdown: uses marked.parseInline() + <span> for valid HTML in h2/p contexts - Switch hero VMarkdown usages to inline prop instead of d-inline class - Add JSDoc to containerStyle() computed with @returns documentation - Update markdown.spec.js: mock DOMPurify, cover inline mode and parseInline fallback
PierreBrisorgueil
force-pushed
the
fix/hero-vertical-centering
branch
from
2f8e212 to
163a4d0
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/lib/plugins/tests/markdown.spec.js (1)
35-127:⚠️ Potential issue | 🟡 MinorAdd JSDoc headers to the modified test callbacks.
Several modified callbacks in this range (
beforeEachand multipleithandlers) still lack the required JSDoc blocks. Please add a one-line description and@returns(plus@paramwhere applicable) for each modified function.As per coding guidelines: "
**/*.{js,ts,vue}: Every new or modified function must have a JSDoc header with one-line description,@paramfor each argument, and@returnsfor any non-void return value (always include@returnsfor async functions)`."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/plugins/tests/markdown.spec.js` around lines 35 - 127, Add a one-line JSDoc header to every modified test callback (the beforeEach handler and each it(...) callback in this file) including a brief description, an `@returns` tag (describe void or Promise if async), and `@param` entries when the callback accepts arguments; specifically update the beforeEach function and the it handlers that reference parseMock/parseInlineMock and the VMarkdown render checks so each has a one-line description and the required `@returns/`@param tags per the repo guideline.
🧹 Nitpick comments (1)
src/modules/home/components/home.hero.component.vue (1)
63-68: Guard the heading/paragraph wrappers to avoid empty semantic elements.
h2andpalways render even whentitle/subtitleis empty, becausev-ifis onVMarkdownonly. Movingv-ifto wrappers avoids empty nodes and spacing artifacts.Suggested template adjustment
-<h2 class="mb-5 font-weight-bold text-headline-large text-md-display-medium blur-title"> - <VMarkdown v-if="title" :source="title" inline /> -</h2> -<p class="mb-10 font-weight-medium text-body-large text-md-headline-small blur-subtitle"> - <VMarkdown v-if="subtitle" :source="subtitle" inline /> -</p> +<h2 v-if="title" class="mb-5 font-weight-bold text-headline-large text-md-display-medium blur-title"> + <VMarkdown :source="title" inline /> +</h2> +<p v-if="subtitle" class="mb-10 font-weight-medium text-body-large text-md-headline-small blur-subtitle"> + <VMarkdown :source="subtitle" inline /> +</p> -<h2 class="mb-5 font-weight-bold text-headline-large text-md-display-medium"><VMarkdown v-if="title" :source="title" inline /></h2> -<p class="mb-10 font-weight-medium text-body-large text-md-headline-small"><VMarkdown v-if="subtitle" :source="subtitle" inline /></p> +<h2 v-if="title" class="mb-5 font-weight-bold text-headline-large text-md-display-medium"><VMarkdown :source="title" inline /></h2> +<p v-if="subtitle" class="mb-10 font-weight-medium text-body-large text-md-headline-small"><VMarkdown :source="subtitle" inline /></p>Also applies to: 98-99
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/home/components/home.hero.component.vue` around lines 63 - 68, The h2 and p wrappers always render even when title/subtitle are empty because v-if is placed on the VMarkdown child; move the conditional to the wrappers so the semantic elements are not emitted when empty — e.g., change the h2 to <h2 v-if="title" ...> with VMarkdown inside (remove v-if from VMarkdown) and similarly change the p to <p v-if="subtitle" ...>; apply the same change for the other occurrence that uses VMarkdown around lines showing the subtitle/title (the second pair at the other occurrence).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/lib/plugins/tests/markdown.spec.js`:
- Around line 35-127: Add a one-line JSDoc header to every modified test
callback (the beforeEach handler and each it(...) callback in this file)
including a brief description, an `@returns` tag (describe void or Promise if
async), and `@param` entries when the callback accepts arguments; specifically
update the beforeEach function and the it handlers that reference
parseMock/parseInlineMock and the VMarkdown render checks so each has a one-line
description and the required `@returns/`@param tags per the repo guideline.
---
Nitpick comments:
In `@src/modules/home/components/home.hero.component.vue`:
- Around line 63-68: The h2 and p wrappers always render even when
title/subtitle are empty because v-if is placed on the VMarkdown child; move the
conditional to the wrappers so the semantic elements are not emitted when empty
— e.g., change the h2 to <h2 v-if="title" ...> with VMarkdown inside (remove
v-if from VMarkdown) and similarly change the p to <p v-if="subtitle" ...>;
apply the same change for the other occurrence that uses VMarkdown around lines
showing the subtitle/title (the second pair at the other occurrence).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (16)
package.jsonsrc/config/defaults/development.jssrc/lib/plugins/markdown.jssrc/lib/plugins/tests/markdown.spec.jssrc/modules/app/app.vuesrc/modules/core/components/core.footer.component.vuesrc/modules/core/components/core.header.component.vuesrc/modules/home/components/home.gallery.component.vuesrc/modules/home/components/home.hero.component.vuesrc/modules/home/components/home.social.component.vuesrc/modules/home/components/home.statistics.component.vuesrc/modules/home/components/home.steps.component.vuesrc/modules/home/components/utils/home.content.component.vuesrc/modules/home/components/utils/home.dynamicIsland.component.vuesrc/modules/home/components/utils/home.img.component.vuesrc/modules/home/views/home.view.vue
🚧 Files skipped from review as they are similar to previous changes (10)
- src/modules/home/components/utils/home.dynamicIsland.component.vue
- src/modules/home/components/utils/home.img.component.vue
- src/modules/home/views/home.view.vue
- src/modules/core/components/core.footer.component.vue
- src/modules/home/components/home.gallery.component.vue
- src/modules/core/components/core.header.component.vue
- package.json
- src/modules/app/app.vue
- src/config/defaults/development.js
- src/modules/home/components/utils/home.content.component.vue
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
overlapprop to hero, migrateVMarkdownfromspantodiv, migrate all Vuetify 3 typography classes to Vuetify 4 MD3 naming across 11 filestext-h1…text-h6,text-body-1/2,text-captionetc. no longer exist, causing text to render at browser default size. Additionally the hero content was never truly vertically centered (v-rowlackedfill-height) andVMarkdownproduced invalid HTML (block<p>inside inline<span>) breaking style inheritance.Scope
home,core,app,lib/pluginsyes—VMarkdownplugin (lib/plugins/markdown.js) changed fromspantodiv; typography class change is global but purely cosmetic/namingmedium— Vuetify 4 typography rename touches 11 files;VMarkdownrender change is a semantic fix with test coverage updatedValidation
npm run lintnpm run test:unitnpm run buildGuardrails check
.env*,secrets/**, keys, tokens)Optional: Infra/Stack alignment details
Before vs After (key changes only)
v-rowauto height,align=centerno-opv-row.fill-height— centering worksv-col(-15vh/-25vh)padding-top: 65pxonv-containeroverlappropoverlap: [Boolean, String, Object]— usescontainerStylepadding-bottomVMarkdownelement<span>(invalid HTML with inner<p>)<div>(valid, correct style inheritance)text-h2,text-body-1,text-caption…text-display-medium,text-body-large,text-label-small…VMarkdowncomponentVMarkdowngetsd-inlineonlyhome.content.componentpattern2a85a652Notes for reviewers
homemodule for any Vuetify 3 typography classesSummary by CodeRabbit
New Features
Style
Tests