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.

Sign up for GitHub

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

Jump to bottom

ui: [suggestion] "opt-in" header minor styling tweaks #12590

Closed
cppforlife opened this issue Dec 28, 2016 · 2 comments
Closed

ui: [suggestion] "opt-in" header minor styling tweaks #12590

cppforlife opened this issue Dec 28, 2016 · 2 comments
Assignees
@mrtracy
Labels
C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) C-question A question rather than an issue. No code/spec/doc change needed. O-community Originated from the community S-3-ux-surprise Issue leaves users wondering whether CRDB is behaving properly. Likely to hurt reputation/adoption.
Milestone
1.0

Comments

@cppforlife
Copy link

imho it would be better:

  • to insert opt-in header above all other elements instead of overlaying it
    • would not hide any content -- currently hides cluster button
  • only show header once appropriate info is loaded (to avoid flashing on page refresh)
    • sometimes page load is slow so there is time between header being visible and then suddenly disappearing
@a-robinson
Copy link
Contributor

Thanks for the feedback, @cppforlife

@kuanluo

@knz knz added the C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) label Dec 29, 2016
@knz knz changed the title [suggestion] "opt-in" header minor styling tweaks ui: [suggestion] "opt-in" header minor styling tweaks Dec 29, 2016
@knz knz added the S-3-ux-surprise Issue leaves users wondering whether CRDB is behaving properly. Likely to hurt reputation/adoption. label Dec 29, 2016
@kuanluo
Copy link

kuanluo commented Dec 29, 2016 •
edited
Loading

Thanks for the feedback, @cppforlife!

  1. would be fixed in an upcoming ui update.
    We will look into 2) to make sure it's also fixed in the update. cc @maxlang

@petermattis petermattis added this to the 1.0 milestone Feb 23, 2017
@kuanluo kuanluo assigned mrtracy and unassigned kuanluo Apr 11, 2017
mrtracy pushed a commit to mrtracy/cockroach that referenced this issue Apr 13, 2017
ui: Alert Refactoring, remove most Banner Alerts
1f9a1cc 
This is a significant refactoring of the systems previously used to display
banner-type notifications.

+ All logic on whether or not to display alerts has been moved to a series of
selectors in `redux/alerts.ts`. After many factoring attempts, using a selector
for each alert seemed like the appropriate design based on current requirements.
+ A new sync service has been added in `alerts.ts`, which is responsible for
loading data from the server which is needed to display alerts. This was chosen
because loading this data using the same method as other components (e.g. how
metric data is loaded) was proving tedious given the nature of alert-relevant
information (only needs to be loaded once, a variety of different settings).
+ Most alerts have been moved from Banners into a dismissable panel which is
displayed on the cluster page. This makes them significantly less disruptive.
+ Significant testing has been added for the alert display logic and the alert
sync service.

Not in this issue:

+ The "cluster disconnected" banner has not been modified, it still displays at
its previous position. The styling of this banner will be changed in a
subsequent pull request, but it will remain visible across all pages because it
is relevant to all pages.

Additional Refactoring changes:

+ Renamed `AdminUIState.ui` -> `AdminUIState.localSettings` to make its purpose
more clear.

Resolves cockroachdb#12890
Resolves cockroachdb#12590
mrtracy pushed a commit to mrtracy/cockroach that referenced this issue Apr 17, 2017
ui: Alert Refactoring, remove most Banner Alerts
d40bddf 
This is a significant refactoring of the systems previously used to display
banner-type notifications.

+ All logic on whether or not to display alerts has been moved to a series of
selectors in `redux/alerts.ts`. After many factoring attempts, using a selector
for each alert seemed like the appropriate design based on current requirements.
+ A new sync service has been added in `alerts.ts`, which is responsible for
loading data from the server which is needed to display alerts. This was chosen
because loading this data using the same method as other components (e.g. how
metric data is loaded) was proving tedious given the nature of alert-relevant
information (only needs to be loaded once, a variety of different settings).
+ Most alerts have been moved from Banners into a dismissable panel which is
displayed on the cluster page. This makes them significantly less disruptive.
+ Significant testing has been added for the alert display logic and the alert
sync service.

Not in this issue:

+ The "cluster disconnected" banner has not been modified, it still displays at
its previous position. The styling of this banner will be changed in a
subsequent pull request, but it will remain visible across all pages because it
is relevant to all pages.

Additional Refactoring changes:

+ Renamed `AdminUIState.ui` -> `AdminUIState.localSettings` to make its purpose
more clear.

Resolves cockroachdb#12890
Resolves cockroachdb#12590
@jordanlewis jordanlewis added C-question A question rather than an issue. No code/spec/doc change needed. O-community Originated from the community and removed O-deprecated-community-questions labels Apr 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@mrtracy mrtracy

Labels
C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) C-question A question rather than an issue. No code/spec/doc change needed. O-community Originated from the community S-3-ux-surprise Issue leaves users wondering whether CRDB is behaving properly. Likely to hurt reputation/adoption.
Projects
None yet
Milestone
1.0
Development

No branches or pull requests
9 participants
@jordanlewis @cppforlife @knz @kuanluo @maxlang @dianasaur323 @petermattis @mrtracy @a-robinson