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

feat: enable list pages [DET-3705] #992

Merged
merged 6 commits into from
Aug 10, 2020

Conversation

hkang1
Copy link
Contributor

@hkang1 hkang1 commented Aug 1, 2020

Description

UPDATE: PR 991 landed
This PR needs to land after PR 991

Screen Shot 2020-08-01 at 2 32 12 PM

Screen Shot 2020-08-01 at 2 33 04 PM

Test Plan

Commentary (optional)

Copy link
Member

@hamidzr hamidzr left a comment

Choose a reason for hiding this comment

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

This is going to be great although I think we still need a few other tickets before we can proceed with this :)

probably for other PRs:

  • we can't create notebooks
  • tensorboard sources missing

webui/elm/src/Main.elm Outdated Show resolved Hide resolved
webui/react/src/routes/index.ts Outdated Show resolved Hide resolved
webui/react/src/routes/index.ts Show resolved Hide resolved
webui/tests/cypress/integration/02-experiments.spec.ts Outdated Show resolved Hide resolved
@hamidzr hamidzr assigned hkang1 and unassigned hamidzr Aug 4, 2020
@hkang1 hkang1 assigned hamidzr and unassigned hkang1 Aug 4, 2020
Copy link
Member

@hamidzr hamidzr left a comment

Choose a reason for hiding this comment

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

This looks good to me and the tests are failing as expected. Would be good to check it out after all the dependencies are merged in. to see the launch notebook and dropdown action in action.

I think we are skipping at least one test unintentionally

webui/tests/cypress/integration/02-experiments.spec.ts Outdated Show resolved Hide resolved
webui/tests/cypress/integration/02-experiments.spec.ts Outdated Show resolved Hide resolved
webui/tests/cypress/integration/02-experiments.spec.ts Outdated Show resolved Hide resolved
@hamidzr hamidzr assigned hkang1 and unassigned hamidzr Aug 4, 2020
@hkang1 hkang1 force-pushed the 3705-enable-list-pages branch 5 times, most recently from d1c18c1 to 4d5e216 Compare August 5, 2020 17:49
@hkang1 hkang1 assigned hamidzr and unassigned hkang1 Aug 5, 2020
@hkang1 hkang1 requested a review from hamidzr August 7, 2020 17:54
@hkang1 hkang1 assigned armandmcqueen and unassigned hamidzr Aug 7, 2020
Copy link
Contributor

@armandmcqueen armandmcqueen left a comment

Choose a reason for hiding this comment

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

I didn't review the Elm portion, but otherwise looks good to me

@@ -100,6 +100,9 @@
.icon-star::before {
content: "\e90a";
}
.icon-tasks::before {
content: "\e920";
Copy link
Contributor

@armandmcqueen armandmcqueen Aug 10, 2020

Choose a reason for hiding this comment

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

Why do we use ::before here?

Copy link
Member

Choose a reason for hiding this comment

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

pseudoelements are common for inserting text through css if that's what you're asking : )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

great question: it's a pattern typically used to display icons via font icon using a pseudo-element ::before. Pseudo elements are reserved for visual elements only and ignored for things like accessibility (like aria-label and title)

Copy link
Member

@hamidzr hamidzr left a comment

Choose a reason for hiding this comment

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

elm side looks good to me 🎉 🚢 🚀

@armandmcqueen armandmcqueen assigned hamidzr and hkang1 and unassigned armandmcqueen and hamidzr Aug 10, 2020
@hkang1 hkang1 merged commit 45034bd into determined-ai:master Aug 10, 2020
@hkang1 hkang1 deleted the 3705-enable-list-pages branch August 10, 2020 20:42
@dannysauer dannysauer added this to the 0.13.0 milestone Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants