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(settings): categorize user settings #793

Merged
merged 24 commits into from
Jan 11, 2023

Conversation

tthvo
Copy link
Member

@tthvo tthvo commented Jan 6, 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

Fixes: #781
Depends on #789

Description of the change:

  • Categorize user settings (similar to OCP console). Settings can be ordered based using an order index.
  • Enable flip and add height constraints for setting menus.
  • Fix nested forms, anchors warnings.
  • Tests for setting page.

Motivation for the change:

See #781

How to manually test:

  1. Run CRYOSTAT_IMAGE=quay.io... sh smoketest.sh...
  2. Go to setting page to see new changes.

Screenshots

new-setting-page

@tthvo tthvo added the feat New feature or request label Jan 6, 2023
@tthvo tthvo requested review from andrewazores and maxcao13 January 6, 2023 18:42
@mergify mergify bot added the safe-to-test label Jan 6, 2023
@tthvo
Copy link
Member Author

tthvo commented Jan 6, 2023

What do you think? I will start on writing tests if this could be accepted :D Just unfortunately, allowing flip on menu causes the snapshots to fail so we need to remove them.

@github-actions
Copy link

github-actions bot commented Jan 6, 2023

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-793-3990b3fe3810bf1584181e9f4b27f7073d8798e6 sh smoketest.sh

@github-actions
Copy link

github-actions bot commented Jan 6, 2023

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-793-0703229ca2339794237383f403de37c8b3e763e6 sh smoketest.sh

@maxcao13
Copy link
Member

maxcao13 commented Jan 6, 2023

I like it, it's very nice!

I don't know if this is just me, but I seem to like the Dashboard coming first before Notification and Messages b/c of alphabetical order but because General and Advanced are "special" cases, I would leave those.

Also, is it just me or did the DEVELOPMENT dropdown in the Feature Level disappear?

@tthvo
Copy link
Member Author

tthvo commented Jan 6, 2023

I don't know if this is just me, but I seem to like the Dashboard coming first before Notification and Messages b/c of alphabetical order but because General and Advanced are "special" cases, I would leave those.

Thanks I was thinking of putting setting groups that might be more frequently visited to the top. Language & Region is also defined but hidden. I will show it when #789 is merged.

@tthvo
Copy link
Member Author

tthvo commented Jan 6, 2023

Also, is it just me or did the DEVELOPMENT dropdown in the Feature Level disappear?

Still seeing it tho...

Screenshot from 2023-01-06 14-13-38

@andrewazores
Copy link
Member

image

Development setting seems to be missing for me too.

@andrewazores
Copy link
Member

I tried in different browsers and in private/incognito modes as well to avoid any effects where it could be from some change to the localstorage format, but in all cases it was the same.

@andrewazores
Copy link
Member

Anyway other than that I think this looks great.

@github-actions github-actions bot removed the dependent label Jan 7, 2023
@github-actions
Copy link

github-actions bot commented Jan 7, 2023

This PR/issue depends on:

@tthvo
Copy link
Member Author

tthvo commented Jan 7, 2023

Ahh I remember it now. DEVELOPMENT is hidden is production build if we use the generated image from github action. This is intended right?

if (!process.env.CRYOSTAT_AUTHORITY) {

@andrewazores
Copy link
Member

Ha. Right, good catch. Didn't even think of that when I pulled the GH pkgs PR preview image.

Looks great. Still need to add more tests?

@tthvo
Copy link
Member Author

tthvo commented Jan 9, 2023

Looks great. Still need to add more tests?

Oh yess! I gonna take this chance to add tests for setting panels. Also need to rebase and move language picker to setting. I will put it as draft for now.

@tthvo tthvo marked this pull request as draft January 9, 2023 06:18
@tthvo tthvo force-pushed the categorize-settings branch from 0703229 to 854f7e7 Compare January 9, 2023 06:41
@github-actions
Copy link

github-actions bot commented Jan 9, 2023

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-793-854f7e7e18a2466246ac832935d01a296fbcb07f sh smoketest.sh

@tthvo tthvo force-pushed the categorize-settings branch from 854f7e7 to 17f50ce Compare January 9, 2023 11:52
@github-actions
Copy link

github-actions bot commented Jan 9, 2023

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-793-17f50cee4a6f2b0d1a294b2a5da74d65d2c6520d sh smoketest.sh

@github-actions
Copy link

github-actions bot commented Jan 9, 2023

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-793-fb3b893caa2cc71d473e40b8188ba00a25b90f43 sh smoketest.sh

@tthvo tthvo force-pushed the categorize-settings branch from fb3b893 to e0db724 Compare January 10, 2023 12:56
@github-actions
Copy link

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-793-e0db724c0fcb91e0b7c1399f25dcaa205ca2ea65 sh smoketest.sh

@tthvo
Copy link
Member Author

tthvo commented Jan 10, 2023

Ready for review again....Changes since last comments:

  • Move language select to setting page + comment out zh field so that its not shown as an option.
  • Wrap setting components in FeatureFlag to hide and show base on current feature level.
    • Except for PRODUCTION-leveled settings, others will have a compact label next to its title (try Language setting).
    • Only show tabs with at least 1 setting that satisfy current feature level.
  • Expand setting page to fill available vertical space (avoid jumps and make it similar to OCP console).
  • Add unit tests for setting components.

Got a bit issue with circular deps in Settings.test.tsx tests, so I disable eslint import for that file for now.

Screenshots:

latest-design

language-beta

@tthvo tthvo marked this pull request as ready for review January 10, 2023 13:04
@tthvo tthvo force-pushed the categorize-settings branch from e0db724 to 2ec3fcc Compare January 10, 2023 13:16
@github-actions
Copy link

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-793-044eb08ae43f69b0a742dda0771363f1841351f0 sh smoketest.sh

@github-actions
Copy link

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-793-e7186ef0c0283debdd5a6ad2cb4ab0dba63a5ea8 sh smoketest.sh

@andrewazores andrewazores merged commit 8d1a321 into cryostatio:main Jan 11, 2023
@tthvo tthvo deleted the categorize-settings branch January 11, 2023 20:08
@tthvo tthvo mentioned this pull request Jan 23, 2023
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[Request] User perferences should be categorized
3 participants