Skip to content
This repository has been archived by the owner on Jan 2, 2025. It is now read-only.

Refactored main page for UI refresh #118

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Refactored main page for UI refresh #118

wants to merge 1 commit into from

Conversation

hrfmartins
Copy link
Collaborator

No description provided.

@hrfmartins hrfmartins requested a review from davidzwa July 2, 2023 22:07
@davidzwa
Copy link
Contributor

davidzwa commented Jul 4, 2023 •
edited
Loading

Very nice work @hrfmartins!! My feedback below is quite direct, it's with the best intention in mind 😃

  • Love the side scrolling animation

  • Very nice mobile responsiveness, this will help a lot!

  • Are images compressed for quicker loading? We can work on this in the future. The farm/github jpegs are quite big, to give an example.

  • Scroll bars appear (2.5k screen):
    image

  • Background image shows lack of anti-aliasing:
    image
    The transparency issue comes from the lines being too thin, or the color of the lines either too dark (not contrasting enough) or too visible (too much contrast) so it is a bit distracting. Maybe I'm biased on the latter part as my eyes are used to the previous black background.
    Also, I would apply a rectangular/radial transparency gradient so the background fades away on the sides.

  • Documentation does not link to docs.fdm-monster.net

  • Demo link is not presented on the top buttons

  • Background does not repeat
    image

  • image
    FDM Monster is not just a "good solution", its a scalable solution based on OctoPrint😉
    Hobbie typo. I would say "suitable for hobby and commercial farms alike", not simply hobby farm.

  • "Interested?" Should be title "Getting started" and lead to "getting started" / "installation" in the docs, not to the demo. I would create a separate tile for the demo titled "See FDM Monster in action"

  • Demo could use a more zoomed-in screenshot (FDMM's contrast is not ideal for screenshots)

  • Mobile view - last tile does not always seem to appear in Edge mobile test mode, I have not tested on mobile itself
    image
    image

  • I'm missing an arrow to go back to the top

I think after these fixes the PR is good to go!

davidzwa
davidzwa reviewed Jul 4, 2023
View reviewed changes
package.json
"webfontloader": "1.6.28"
},
"devDependencies": {
"@babel/types": "7.22.5",
"@iconify/vue": "^4.1.1",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please pin the version (to do this just remove any ^ or ~.

davidzwa
davidzwa reviewed Jul 4, 2023
View reviewed changes
src/components/MarketingSection.vue
<div>
<div class="text-4xl mb-4">Open Source</div>
<div>
The project is entirely open-source, <a href="https://github.com/fdm-monster" class="text-red-500">check it out on GitHub ❤️</a>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not entirely open-source: it's licensed under AGPL v3.

davidzwa
davidzwa reviewed Jul 4, 2023
View reviewed changes
public/github.png
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe github in dark mode?

davidzwa
davidzwa reviewed Jul 4, 2023
View reviewed changes
public/farm.jpeg
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be good to reference where you got this image from... it might not be ours to use. I recognize this from Prusa.

davidzwa
davidzwa reviewed Jul 4, 2023
View reviewed changes
package.json
"@types/jsdom": "21.1.1",
"@types/node": "18.16.19",
"@types/webfontloader": "1.6.35",
"@vitejs/plugin-vue": "4.2.3",
"@vue/tsconfig": "0.1.3",
"autoprefixer": "^10.4.14",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not see autoprefixer configured. Maybe this is missing?

@hrfmartins hrfmartins marked this pull request as draft November 18, 2023 13:10
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@davidzwa davidzwa davidzwa left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@hrfmartins @davidzwa