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

tweak: admin improvements #6391

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Conversation

briangregoryholmes
Copy link
Contributor

@briangregoryholmes briangregoryholmes commented Jan 10, 2025

https://www.notion.so/rilldata/Platform-Copy-label-improvements-10bba33c8f5780029a3efdc42b419c9c?pvs=4

Additional changes:

  • Updates BasicTable implementation for improved usability on narrow screens and to satisfy layouts that VirtualTable, PivotTable or Table can't easily satisfy
  • Updates admin page layout to stack vertically on narrower screens
  • Top navigation bar is pinned to top when scrolling
  • Adds animated tab underline to better communicate the active tab
  • Consolidates "Status" and "Error" in Resource Status Table and adds loading spinner
  • Integrates "click to copy" functionality for error message into status icon
  • Removes maxWidthOverride prop structure
  • Updates admin vs common tabs implementation to improve code clarity
Screenshot 2025-01-10 at 2 54 39 PM Screenshot 2025-01-10 at 2 54 08 PM Screenshot 2025-01-10 at 2 53 59 PM Screenshot 2025-01-10 at 2 53 47 PM Screenshot 2025-01-10 at 2 53 36 PM Screenshot 2025-01-10 at 2 53 43 PM

Closes: #6358

@jkhwu
Copy link
Contributor

jkhwu commented Jan 10, 2025

@briangregoryholmes I'm having trouble getting devtool to work but based on the screenshots, could you please update the underline of the selected tab to be gray-800 rather than the primary color? I think it should match the color of the tab text.

@jkhwu
Copy link
Contributor

jkhwu commented Jan 10, 2025

@briangregoryholmes could you please add @ericokuma as a reviewer to this PR since he is the product owner for the associated PRD?

@briangregoryholmes briangregoryholmes requested review from ericokuma and removed request for jkhwu January 10, 2025 20:50
@jkhwu
Copy link
Contributor

jkhwu commented Jan 10, 2025

@briangregoryholmes I'm having trouble getting devtool to work but based on the screenshots, could you please update the underline of the selected tab to be gray-800 rather than the primary color? I think it should match the color of the tab text.

I'm retracting this comment now that I've looked at the actual PR. Let's keep it primary.

@ericokuma
Copy link

ericokuma commented Jan 10, 2025

Thanks @briangregoryholmes!

Couple of UXQA items:

  • When there are no dashboards, omit the page label "Project dashboards"
  • When there are no reports, omit the page label "Project reports"
  • When there are no alerts, omit the page label "Project alerts"
  • For Org level settings, let's label as "Organization settings"
  • Empty state of tables seem to have been broken
    Currently in production:
    image

In Staging:
Screenshot 2025-01-10 at 2 20 20 PM

@ericokuma
Copy link

In addition, looks like the settings and project status panels are pinched in by accident?

Project Status

Production:
image

Staging:
image

Settings

Production:
Screenshot 2025-01-10 at 2 26 42 PM
Staging:
Screenshot 2025-01-10 at 2 26 55 PM

Not sure if this came from this PR but let's revert back to the previous widths for those elements

@briangregoryholmes
Copy link
Contributor Author

briangregoryholmes commented Jan 11, 2025

Addressed all.

As for the pinching, the problem is actually that those pages previously did not have a functional max width. They were defined only by strict x-axis padding. As such, there's really no previous size to return to. Because I don't think that was intended with the design, I've widened the allowed width for those pages to 1100 pixels.

Copy link
Contributor

@ericpgreen2 ericpgreen2 left a comment

Choose a reason for hiding this comment

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

Wow, this looks way better! Approving from a general code perspective. I'll let @ericokuma give the final approval as Product Owner. And @lovincyrus please do look at the environment variable table.

@ericokuma
Copy link

Looks amazing @briangregoryholmes! Great job!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Copy/label improvements
4 participants