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

Instance status configuration #8096

Merged
merged 22 commits into from
Feb 17, 2022
Merged

Instance status configuration #8096

merged 22 commits into from
Feb 17, 2022

Conversation

paolodamico
Copy link
Contributor

@paolodamico paolodamico commented Jan 17, 2022

Changes

Nice confirmation screen before saving

Warning when changing recordings TTL (@rcmarron)

Info when changing email settings

How did you test this code?

  • Open the instance settings page with your user is_staff = True.
  • Test updating single setting.
  • Test updating multiple settings.
  • Test updating boolean, number & string values.

@posthog-bot
Copy link
Contributor

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the stale label – otherwise this will be closed in another week.

@posthog-bot
Copy link
Contributor

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the stale label – otherwise this will be closed in another week.

@paolodamico paolodamico marked this pull request as ready for review February 9, 2022 12:07
Copy link
Member

@Twixes Twixes 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 great! Just a few rough edges

<>
<LemonSpacer />
<PageButton
title="Instance settings"
Copy link
Member

Choose a reason for hiding this comment

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

I would not have guessed that this is reachable from this button:
Screen Shot 2022-02-15 at 15 36 23
Perhaps it should be renamed from "System status" to "Manage instance"? (though seems like this will require making SitePopover 1 rem wide)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well as we now have an additional menu item if the user is staff, I think it's fine to keep this way. If you're accessing the page from the site popover, it makes sense that you want to focus on the system status.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, honestly I didn't notice the sidebar button. This raises more questions – it's kind of weird:

  • there are now two buttons that take you to this page – one in the sidebar, and one in the popover
  • the sidebar button takes you to instance settings… but that's the third tab of the page, not the first, which is a surprising destination for a pattern that's meant for top-level pages
  • "Settings" is just one of a few tabs – a sidebar item should cover all tabs of the page it leads too (otherwise the highlighting is confusing when you navigate to another page tab for example)

If we go the sidebar item route, I strongly propose making instance settings their own page, with status/metrics reachable from the popover like it is currently.

frontend/src/scenes/urls.ts Outdated Show resolved Hide resolved
@paolodamico paolodamico requested a review from Twixes February 16, 2022 16:00
@paolodamico
Copy link
Contributor Author

Thanks for the thorough review @Twixes, all feedback addressed, ready for a final look.

Copy link
Member

@Twixes Twixes left a comment

Choose a reason for hiding this comment

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

All good except got some doubts about the navigation patterns in use

<>
<LemonSpacer />
<PageButton
title="Instance settings"
Copy link
Member

Choose a reason for hiding this comment

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

Ah, honestly I didn't notice the sidebar button. This raises more questions – it's kind of weird:

  • there are now two buttons that take you to this page – one in the sidebar, and one in the popover
  • the sidebar button takes you to instance settings… but that's the third tab of the page, not the first, which is a surprising destination for a pattern that's meant for top-level pages
  • "Settings" is just one of a few tabs – a sidebar item should cover all tabs of the page it leads too (otherwise the highlighting is confusing when you navigate to another page tab for example)

If we go the sidebar item route, I strongly propose making instance settings their own page, with status/metrics reachable from the popover like it is currently.

frontend/src/layout/navigation/SideBar/SideBar.tsx Outdated Show resolved Hide resolved
frontend/src/scenes/instance/SystemStatus/index.tsx Outdated Show resolved Hide resolved
@paolodamico
Copy link
Contributor Author

Thanks @Twixes! Alright I've decided just to rename the page to make it consistent, and the sidebar link now opens the first tab. We can iterate later if it makes sense.

@paolodamico paolodamico merged commit 15ce33e into master Feb 17, 2022
@paolodamico paolodamico deleted the instance-status-conf branch February 17, 2022 19:42
EDsCODE added a commit that referenced this pull request Feb 18, 2022
* master:
  hobby: Wait for ClickHouse and for Postgres before starting (#8686)
  Part 2: Deprecate old tags and upgrade to new tags Backend (#8529)
  Remove flake8-commas (#8695)
  Update Breakdown props to use filter groups (#8679)
  Automatically switch to the right project if possible (#8681)
  Super Lazy VMs (#8609)
  .github/workflows/ci-backend.yml: fix flake8 config (#8676)
  Fix recording page refresh loop (#8685)
  Instance status configuration (#8096)
@clarkus
Copy link
Contributor

clarkus commented Feb 23, 2022

This PR resulted in adding an instance level navigation item to the project navigation. By design, everything in that menu should be project navigation. We already link to this via the utilities menu. Likewise a lot of the modals, banners, etc are using really long form values and they don't align to our other components of the same type.

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.

Email setup from the PostHog app
4 participants