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(dashboard): dashboard card catalog #937

Merged
merged 12 commits into from
Apr 6, 2023

Conversation

tthvo
Copy link
Member

@tthvo tthvo commented Apr 5, 2023

Welcome to Cryostat! 👋

Before contributing, make sure you have:

  • Read the contributing guidelines
  • Linked a relevant issue which this PR resolves
  • Linked any other relevant issues, PR's, or documentation, if any
  • Resolved all conflicts, if any
  • Rebased your branch PR on top of the latest upstream main branch
  • Attached at least one of the following labels to the PR: [chore, ci, docs, feat, fix, test]
  • Signed the last commit: git commit --amend --signoff

Related to #468

Description of the change:

  • Used gallery view for supported card view.
  • Fix JVM detail card (needed a fix height since child node uses flex box). This, however, is a temporary fix. Final refactor should clean this up when we support vertical resize.
  • JFR Metrics is marked as Beta so quickstarts should follow.
  • Remove some unused css rules.

Motivation for the change:

See #727 (comment)

Screenshots

Empty

Screenshot from 2023-04-05 00-49-33

Some cards

Screenshot from 2023-04-04 22-02-33

Catalog

Screenshot from 2023-04-05 01-36-15

@tthvo tthvo added the feat New feature or request label Apr 5, 2023
@github-actions github-actions bot added the needs-triage Needs thorough attention from code reviewers label Apr 5, 2023
@mergify mergify bot added the safe-to-test label Apr 5, 2023
@tthvo tthvo removed the needs-triage Needs thorough attention from code reviewers label Apr 5, 2023
@tthvo
Copy link
Member Author

tthvo commented Apr 5, 2023

Compared to the original comments, there are not actual card previews tho. This requires us to bundle more screenshots so its not very ideal as the number of possible card grows.

We can try design some icons for it but for now, its just a general icon. Labels are also supported so that should also help explain.

@tthvo tthvo added the chore Refactor, rename, cleanup, etc. label Apr 5, 2023
@tthvo tthvo changed the title feat(dashboard): gallery view for supported cards feat(dashboard): dashboard card catalog Apr 5, 2023
@github-actions
Copy link

github-actions bot commented Apr 5, 2023

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-937-4e2f591ce1ca237dcb5eae1802fa1db3559405d4 sh smoketest.sh

@tthvo
Copy link
Member Author

tthvo commented Apr 5, 2023

Check with darkmode:

Screenshot from 2023-04-04 22-25-29

The header is a little dark but overall everything looks fine.

Screenshot from 2023-04-05 01-38-14

@github-actions
Copy link

github-actions bot commented Apr 5, 2023

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-937-c6554337935d497b2e446b465a94f0ac44eb74ab sh smoketest.sh

@github-actions
Copy link

github-actions bot commented Apr 5, 2023

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-937-a747a891f8e1f5e5c7bda7964c0a41ca4f62ea6a sh smoketest.sh

@github-actions
Copy link

github-actions bot commented Apr 5, 2023

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-937-d33d2b46decbd22b350f28eeafc50e09dc04a137 sh smoketest.sh

@github-actions
Copy link

github-actions bot commented Apr 5, 2023

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-937-a5b99269c4b5ca4413e25e60eb249ba2d2a64021 sh smoketest.sh

@andrewazores
Copy link
Member

Compared to the original comments, there are not actual card previews tho. This requires us to bundle more screenshots so its not very ideal as the number of possible card grows.

Maybe not - we could just render the actual components but with fake data.

@tthvo
Copy link
Member Author

tthvo commented Apr 5, 2023

Something like this works? The view is not interactive :)) the elements are there but there is an invisible overlay to cover it, disabling click events.

Screenshot from 2023-04-05 16-26-03

@andrewazores
Copy link
Member

andrewazores commented Apr 5, 2023

Looks perfect functionally. I think ideally the card components themselves should not know anything about "being fake" or not, the fake data should simply be supplied to them from the outside. This could mean via their props or it could mean by wrapping a context provider that provides faked data around the gallery preview card, for example a fake ServiceContext with a fake ApiService that just returns mock data. This could be a refactoring for later if you think it will take a lot of effort.

@github-actions
Copy link

github-actions bot commented Apr 5, 2023

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-937-727263ee9a85c54e756ac4650d553326abb38aa3 sh smoketest.sh

@tthvo
Copy link
Member Author

tthvo commented Apr 5, 2023

I think ideally the card components themselves should not know anything about "being fake" or not, the fake data should simply be supplied to them from the outside.

Yehh this would be very nice! Just that each card has a different set of data and are mostly self-contained now. I will leave it for later refactoring then cuz i might mess up the chart controller.

EDIT: Oh wait actually, from ur latest comments, i can give it a try.

@andrewazores
Copy link
Member

andrewazores commented Apr 5, 2023

Sounds good.

Before I dive into the implementation review, I'm not sure how I feel about placing the AddCard as the first element in the layout. Maybe it's just against what I've become used to, but it feels "in the way" to me when it's first in the list and therefore feels like perhaps the most important element in the list. I think keeping it as last, or perhaps even removing it and just making it into an action button on the new dashboard toolbar could be better. Maybe the AddCard should only appear if the current layout is empty, and otherwise that function is instead provided by a toolbar action button? Not sure if this conforms to PF design guidelines.

@tthvo
Copy link
Member Author

tthvo commented Apr 5, 2023

Actually, i placed the add card first following this example:

https://www.patternfly.org/v4/demos/card-view/react-demos/card-view/

@andrewazores
Copy link
Member

Yea, I'm not sure if I like that PF guideline though. The cards they demo are very simple and static and all the cards are pretty small in comparison to our dashboard.

@tthvo
Copy link
Member Author

tthvo commented Apr 5, 2023

ohh yeh...hmm the toolbar now is for dashboard layout. I will try to see how to put the button there.

@tthvo tthvo force-pushed the add-dashboard-card branch 2 times, most recently from d33d2b4 to 71cc50c Compare April 5, 2023 22:05
@github-actions
Copy link

github-actions bot commented Apr 5, 2023

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-937-71cc50cdee8dbee8dbbf9ed579f5ab9b21fae1b6 sh smoketest.sh

@maxcao13
Copy link
Member

maxcao13 commented Apr 6, 2023

image

I think the dashboard quickstart needs to be updated.

The UI looks really nice!

@maxcao13
Copy link
Member

maxcao13 commented Apr 6, 2023

image

Is it possible to make the height of the modal a bit bigger to make the catalog fit two cards without scrolling on a normal viewport? I tried using height prop to modal at height=50em but for some reason that didn't work. Not sure if there is a way without using css rules and className of modal.

@maxcao13
Copy link
Member

maxcao13 commented Apr 6, 2023

image

Some localization keys missing it seems from some refactoring...

@tthvo
Copy link
Member Author

tthvo commented Apr 6, 2023

Is it possible to make the height of the modal a bit bigger to make the catalog fit two cards without scrolling on a normal viewport? I tried using height prop to modal at height=50em

Yeh I think I can try using percentage height. Tho, there is no guarantee that it will show all cards as it depends on the current browser dimension. However, the grid is scrollable to it should not be too bad.

Some localization keys missing it seems from some refactoring...

Opps. Looking now..

@maxcao13
Copy link
Member

maxcao13 commented Apr 6, 2023

Thoughts on moving the panel content body in the drawer head like this so that there is less white space since there is no header?

image

        <DrawerHead>
        <DrawerPanelBody className="card-catalog__detail-panel-body">
          <Stack hasGutter>
           ...omitted
          </Stack>
        </DrawerPanelBody>
          <DrawerActions>
            <DrawerCloseButton onClick={() => setToViewCard(undefined)} />
          </DrawerActions>
        </DrawerHead>

Maybe could use some negative padding too...

@tthvo
Copy link
Member Author

tthvo commented Apr 6, 2023

Maybe could use some negative padding too...

Negative margin should do the tricks^^

@tthvo
Copy link
Member Author

tthvo commented Apr 6, 2023

Ready for review again :)) Addressed all suggestions now^^ One new thing is that the MBean metric card actually have some moving data points so it looks like its real time.

@github-actions
Copy link

github-actions bot commented Apr 6, 2023

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-937-552c1646092cf3459b0804b929002576953cbcff sh smoketest.sh

Copy link
Member

@maxcao13 maxcao13 left a comment

Choose a reason for hiding this comment

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

Everything looks good to me now, great job!

@andrewazores andrewazores merged commit eb001a7 into cryostatio:main Apr 6, 2023
@tthvo tthvo deleted the add-dashboard-card branch April 6, 2023 22:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Refactor, rename, cleanup, etc. feat New feature or request safe-to-test
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants