Skip to content
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

fix(HelloWorld): modernize & cleanup HelloWorld #67

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

J-Sek
Copy link

@J-Sek J-Sek commented Mar 23, 2025

#66 prompted me to review the generated HelloWorld.vue and it appears it has a potential for improvement. These changes only address code quality, but I might also follow with slight visual tweaks if I find some time this month.

Highlights:

  • removes unnecessary v-overlay and confusing use of v-responsive
  • extracts links and utilizes v-for to make a good first impression

Even if we agree nobody will bother peeking inside, this cleanup just improves maintainability of this repo.

Playground

@J-Sek J-Sek self-assigned this Mar 23, 2025
@johnleider
Copy link
Member

Looks good. If we're going to change this, do you think it would benefit us to explore if the welcome page only utilized built in functionality. Maybe a waste of time, but I think it would be cool if it was possible.

@AndreyYolkin
Copy link

I propose to keep VOverlay to keep only built-in features
Playground

@J-Sek
Copy link
Author

J-Sek commented Apr 3, 2025

Or we could just switch to variant="tonal" instead. I was playing with glow a bit and I don't particularly miss the blue tint. example.

But to finalize this PR, I can adjust the code either way. Options:

  • keep :style=...
  • restore v-overlay
  • or switch to variant="tonal" and drop the tint

@johnleider, you already kinda expressed similar opinion, but I wish to get more clear vote here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants