ArdanaLabs / DanaSwapUI

Other
3 stars 3 forks source link

Landing ejs refactor #74

Closed elite0226 closed 2 years ago

elite0226 commented 2 years ago
leomayleomay commented 2 years ago

@elite0226 would you please help to fix the conflicts and CI failures before QA? cheers

DarthPJB commented 2 years ago

@elite0226 Your branch has removed the 'ardana-landing' module from frontend-landing.

The CI build pipeline is reliant on this to produce the output of the frontend-landing derivation.

As such, this will not evalulate.

DarthPJB commented 2 years ago

So - I've corrected the flake.nix to reflect the renaming of the node module in question - this is throwing up all kinds of NPM errors that I'm unable to debug or fix.

Please ensure that you are able to nix build .#frontend-landing locally before commiting to a non draft PR.

toastal commented 2 years ago

Screen Shot 2022-06-03 at 10 48 00

Video broken without JS. JS is probably not a requirement to play videos.

toastal commented 2 years ago

Screen Shot 2022-06-03 at 10 51 15

The <noscript> I built, while cute, isn't really relevant anymore. JavaScript is not a requirement to view this. You could choose to leave a small banner along the lines of "you'll have a better experience with JavaScript enabled", but honestly that probably isn't needed.

toastal commented 2 years ago

Fundamental issue 1: if there's going to me a big mass of utility classes and you're choosing to go the utility class route, why not just use a library like Tailwind or Tachyons?

Fundamental issue 2: you sure you want to build a flex-based grid system in 2022 without a) considering native display: grid or b) using the grid systems from one of the things raised in issue 1?

toastal commented 2 years ago

Consider just setting a https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Referrer-Policy

toastal commented 2 years ago

https://cssgridgarden.com/

Much the styles could be simplified with grid and gap. Grid is still very accessible to users, and while it doesn't look great on old/defunct browsers, it still works good enough and is perfectly fine for TUI browsers and screen readers.

toastal commented 2 years ago

I highly, highly suggest you try loading the site in a TUI browser ala

$ w3m http://localhost:5000

And it should start being immediately obvious where all caps shouldn't be used. Where alt text is missing--or where it shouldn't exist because the image is purely decorative. What elements need links (or perhaps "View more" should just be a <details>??). There other questions that poses like: why are there two, duplicate navigation headers?

leomayleomay commented 2 years ago

@elite0226 would you please help to address the comments and the failed CI? cheers

toastal commented 2 years ago

@elite0226 Is the CSP in yet?

toastal commented 2 years ago

Carousel noscript

Screen Shot 2022-06-27 at 05 32 54

Carousel with script

Screen Shot 2022-06-27 at 05 33 18

Issues (invoked via npm start @ frontend-landing):

Optional:

leomayleomay commented 2 years ago

@elite0226 the CI build is failed

leomayleomay commented 2 years ago

the following QA feedbacks are collected when testing against https://631093cfa1bdf85f413bc2bc--admirable-snickerdoodle-06e08d.netlify.app

toastal commented 2 years ago

404

If this is on Netlify, we need to make a 404.html (or whatever the use) that is a duplicate of our index.html to get the fallbacks.

toastal commented 2 years ago

broken images

../../../../assets/icons/video-play.svg. Looks like an issue with relative URLs. Removing the relative .. still fails though.

toastal commented 2 years ago

Not required for release, but these raster images look kinda bad on my hiDPI screen.

leomayleomay commented 2 years ago

Finish the QA with URL https://631f2f795103a004f007cd83--admirable-snickerdoodle-06e08d.netlify.app in Chrome/Firefox/Brave, the issues found in the previous round of QA are all fixed

leomayleomay commented 2 years ago

hi Greg, I tested this with Epiphany browser mentioned by Kai, and found weird boxes around images, cc @toastal

image

leomayleomay commented 2 years ago

hi Greg, I tested this with Epiphany browser mentioned by Kai, and found weird boxes around images, cc @toastal

image

this seems to be fixed with https://63274b67654405588e129076--admirable-snickerdoodle-06e08d.netlify.app/

leomayleomay commented 2 years ago
leomayleomay commented 2 years ago

looks great, only two minor issues

leomayleomay commented 2 years ago

@elite0226 I found that there's no way I could close the burger menu once it's open

image

toastal commented 2 years ago

@ryanmatovu I feel this was merged prematurely because I have real, long-term concerns about how the class is checked and changed on the document.body. Can we revert this and fix those issues first? Or open new issues to fix it immediately.

ryanmatovu commented 2 years ago

@toastal we can open new issues to fix it immediately.