Skip to content
This repository was archived by the owner on Jun 3, 2021. It is now read-only.

feat(landing_page): First draft #30

Merged
merged 2 commits into from
Aug 17, 2020
Merged

feat(landing_page): First draft #30

merged 2 commits into from
Aug 17, 2020

Conversation

frazs
Copy link
Contributor

@frazs frazs commented Jul 28, 2020

First draft of the landing page. The functionality and general concept is there, and if desired I can add on some relatively simple things such as SVG icons, but I am not much of a front end developer so this may warrant being passed up to somebody in UI.

Addresses StatCan/aaw-kubeflow-containers#38

@frazs
Copy link
Contributor Author

frazs commented Jul 28, 2020

@frazs
Copy link
Contributor Author

frazs commented Jul 28, 2020

Threw in a quick go at icons, which works great locally but is completely messed in that html preview, so I'm going to take a look into that.

@frazs
Copy link
Contributor Author

frazs commented Jul 28, 2020

Looks to be an issue with the previewer (possibly a caching one - it may be pulling an older version of the css), this works: https://raw.githack.com/StatCan/kubeflow-containers-desktop/landingpage/base/resources/landing_page/index.html

Though my CSS was very fast and loose for a quick proof of concept, with text changed to get things to align better (wrt e.g. centering), so there should still be a better way to set things up with a more explicit structure.

@frazs frazs marked this pull request as draft July 31, 2020 16:20
@frazs
Copy link
Contributor Author

frazs commented Jul 31, 2020

In today's stand-up, @brendangadd said he wanted to go over the HTML for best practices and accessibility before this is pulled in. I'm putting this in Draft until that's done.

For anyone reviewing this, note that one way or another the URLs must be dynamically built (but perhaps it is possible to dynamically build a string that's fed into an <a href ...> if that is what is best for e.g. screen readers). This is because of the dynamic workspace base URL component (the part that is /notebook/namespace/container-name/). Or at least in my attempts to use relative links, they would attach directly to the kubeflow domain, which wouldn't work -- but perhaps I missed something.

@frazs
Copy link
Contributor Author

frazs commented Aug 5, 2020

Rebased to sync up with other recent updates. @wg102 is going to take a look.

@frazs
Copy link
Contributor Author

frazs commented Aug 5, 2020

While there please see if you can fix the "no newline" git warnings (IIRC Saffa had luck just adding a new line at the end) and I'm going to try to figure out why that keeps happening on my end.

@wg102
Copy link

wg102 commented Aug 12, 2020

I modified the code to use 'a' elements instead and verified the navigation using tabs.

@frazs frazs marked this pull request as ready for review August 12, 2020 17:06
@frazs frazs requested a review from brendangadd August 12, 2020 17:06
@frazs
Copy link
Contributor Author

frazs commented Aug 12, 2020

LGTM. @brendangadd?

Copy link
Collaborator

@brendangadd brendangadd left a comment

Choose a reason for hiding this comment

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

In addition to line comments:

  • Run JS through eslint
  • Run HTML, CSS, JS through Prettier

@wg102 wg102 force-pushed the landingpage branch 2 times, most recently from 4b45756 to a611e34 Compare August 17, 2020 13:24
@wg102 wg102 requested a review from brendangadd August 17, 2020 13:42
@brendangadd brendangadd merged commit aa79bba into master Aug 17, 2020
@brendangadd brendangadd deleted the landingpage branch August 17, 2020 15:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants