ui: Release notes signup - Show Alert notification after signup by koorosh · Pull Request #44856 · cockroachdb/cockroach

koorosh · 2020-02-07T13:37:05Z

Resolves: #43912

Existing AlertBox component doesn't fit to new designs from Design system,
so new one is added as a wrapper on top of Ant design component.
The major problem which occur is how to integrate new AlertMessage component
to live with old one and do not affect each other behaviour.
To achieve this:

AlertMessage component (new one) has the same set of properties. As result it will allow easily consume old alert messages by new component.
To preserve old messages without any changes, one additional property is
added to specify which Alert component to show, old or new one.
Current implementation reuses existing logic where we store state of alert in
local settings instead of redux store.

cockroach-teamcity · 2020-02-07T13:37:13Z

This change is

Existing AlertBox component doesn't fit to new designs from Design system, so new one is added as a wrapper on top of Ant design component. The major problem which occur is how to integrate new AlertMessage component to live with old one and do not affect each other behaviour. To achieve this: - AlertMessage component (new one) has the same set of properties. As result it will allow easily consume old alert messages by new component. - To preserve old messages without any changes, one additional property is added to specify which Alert component to show, old or new one. - Current implementation reuses existing logic where we store state of alert in local settings instead of redux store. Release note: None

Create localSetting selector to preserve alert state (open/closed) for email subscription notification. Selector is added to the `bannerAlertsSelector` because it is used to display global alerts. Release note: None

And alerts by default is positioned on the top left and this position is overridden with margin values to put alert on right side. But after user closes alert, it's closing with animation and new margins which make alert jump from right to left side. To fix this, we need to override margins with !important. Release note: None

Release note: None

dhartunian · 2020-02-24T22:13:03Z

pkg/ui/src/views/shared/components/alertMessage/alertMessage.tsx

+// by the Apache License, Version 2.0, included in the file
+// licenses/APL.txt.
+
+import React from "react";


I'm confused as to which new components go in src/views/shared/components and which ones in /src/components? I assumed all new work would go in the second directory.

@dhartunian , I also thought it has to be in /src/components, but this component is implemented in such a way to be compatible (reuse the same interface as another AlertBox component).
This required to use react-router-dom lib and reference to import { AlertInfo, AlertLevel } from "src/redux/alerts" inside component.
With all of this dependencies it doesn't look like pure component so I decided to put in shared components.

dhartunian · 2020-02-24T22:16:54Z

pkg/ui/src/views/shared/components/alertMessage/alertMessage.tsx

+};
+
+export class AlertMessage extends React.Component<AlertMessageProps> {
+  static defaultProps: Partial<AlertMessageProps> = {


why is this defaultProps necessary?

Indeed, not necessary! Has to be cleaned up.

dhartunian · 2020-02-24T22:29:31Z

pkg/ui/src/redux/alerts.ts

  dismiss: ThunkAction<Promise<void>, AdminUIState, void>;
+  // Makes alert to be positioned in the top right corner of the screen instead of
+  // stretching to full width.
+  showAsAlert?: boolean;


can this be a bit more specific? maybe say showAsAlertMessage. This is a bit vague and confusing since the word alert appears everywhere.

dhartunian · 2020-02-24T22:31:09Z

pkg/ui/src/redux/alerts.spec.ts

+        // dismiss alert
+        const alert = emailSubscriptionAlertSelector(state());
+        await alert.dismiss(dispatch, state);
+        const dismissedState = emailSubscriptionAlertLocalSetting.selector(state());


Do you think this should also be openState? It reads a bit weird to dismiss the alert and then check that dismissedState isFalse. What do you think?

Hah, good point! Thank you!

Release note: None

dhartunian · 2020-02-25T20:18:15Z

bors r+

craig · 2020-02-25T20:53:33Z

Build succeeded

GitHub CI (Cockroach)

45143: ui: Release notes signup r=dhartunian a=koorosh Resolves: #43912 Depends on: #44744, #44821, #44856 - [x] WIP. Current branch has two branches merged which haven't been approved/merged in master branch yet. - [x] rebase branch to remove merge commits from branches other than master. Add Release notes signup form to Cluster Overview page right after page title. Release Notes signup view is created in `src/views/dashboard` directory because it will be the main page where we display this view. And Cluster Overview page is a temporary place while Dashboard view doesn't exist. These changes integrate three main parts: ReleaseNotesForm, AlertNotification component and custom analytics saga. 45426: coldata: minor tweak of flat bytes r=yuzefovich a=yuzefovich This commit changes `maybeBackfillOffsets` to update `maxSetIndex` accordingly (this might be a minor performance improvement). In a sense, when we're backfilling offsets, we *are* setting indices to point to empty `[]byte` slices. Also, the logic for `Set` method is slightly refactored. Release note: None 45455: clusterversion: significantly rework cluster version handling r=tbg a=irfansharif We split off ClusterVersion out of pkg/settings/cluster into a dedicated pkg/clusterversion. This is to pave the way for #39182 where we introduce long running version migration infrastructure. We then introduce clusterversion.Handle as a read only view to the active cluster version and this binary's version details. We similarly fold in the actual global cluster version setting into pkg/clusterversion, and enforce all external usage to go through clusterversion.Handle. We can also hide away external usage of the baked in cluster.Binary{,MinimumSupported}Version constants. Instead we have the implementation of clusterversion.Handle allow for tests to override binary and minimum supported versions as needed. Along the way we clean up Settings.Initialized, as it is not really used. We document all the various "versions" in play ("binary version", "minimum supported version", "active version") and change usage of what was previously "serverVersion" to simply be "binaryVersion", because that's what it is. We also clean up the Settings constructors into Test/Non-Test types that differ in cluster version setting initialization behaviour. --- For reviewers: It's probably easiest to start with what pkg/settings/cluster/cluster_settings.go looks like, then following into pkg/clusterversion/cluster_version.go and then pkg/clusterversion/setting.go. --- I still don't like the following detail about our pkg structure: - pkg/settings/cluster depends on it's "parent" pkg, pkg/settings - pkg/settings/cluster depends pkg/clusterversion, which in turn depends on pkg/settings Naively, pkg/settings/cluster should be a top level package, but I'm not making that change in this PR. For now I've renamed the settings.go file to cluster_settings.go. Release note: None 45535: Revert "storage,libroach: update MVCCIncrementalIterator to look at every updated key" r=pbardea a=pbardea Reverts #45163 To stop the errors we're seeing on #45524. Will investigate further once it's off master. Co-authored-by: Andrii Vorobiov <and.vorobiov@gmail.com> Co-authored-by: Yahor Yuzefovich <yahor@cockroachlabs.com> Co-authored-by: irfan sharif <irfanmahmoudsharif@gmail.com> Co-authored-by: Daniel Harrison <daniel.harrison@gmail.com> Co-authored-by: Paul Bardea <pbardea@gmail.com>

45143: ui: Release notes signup r=dhartunian a=koorosh Resolves: #43912 Depends on: #44744, #44821, #44856 - [x] WIP. Current branch has two branches merged which haven't been approved/merged in master branch yet. - [x] rebase branch to remove merge commits from branches other than master. Add Release notes signup form to Cluster Overview page right after page title. Release Notes signup view is created in `src/views/dashboard` directory because it will be the main page where we display this view. And Cluster Overview page is a temporary place while Dashboard view doesn't exist. These changes integrate three main parts: ReleaseNotesForm, AlertNotification component and custom analytics saga. 45426: coldata: minor tweak of flat bytes r=yuzefovich a=yuzefovich This commit changes `maybeBackfillOffsets` to update `maxSetIndex` accordingly (this might be a minor performance improvement). In a sense, when we're backfilling offsets, we *are* setting indices to point to empty `[]byte` slices. Also, the logic for `Set` method is slightly refactored. Release note: None Co-authored-by: Andrii Vorobiov <and.vorobiov@gmail.com> Co-authored-by: Yahor Yuzefovich <yahor@cockroachlabs.com>

koorosh requested a review from a team February 7, 2020 13:37

koorosh added 2 commits February 17, 2020 12:28

ui: Add localSetting selector and actions for email subscription alert

6404cd1

Create localSetting selector to preserve alert state (open/closed) for email subscription notification. Selector is added to the `bannerAlertsSelector` because it is used to display global alerts. Release note: None

koorosh force-pushed the ui-release-notes-signup__alert-notification branch from 437fc01 to 6404cd1 Compare February 17, 2020 10:28

koorosh mentioned this pull request Feb 18, 2020

ui: Release notes signup #45143

Merged

2 tasks

ui: Style Close button for Alert panel

b979307

Release note: None

dhartunian requested changes Feb 24, 2020

View reviewed changes

koorosh added 2 commits February 25, 2020 12:09

ui: Get rid of useless defaultProps for AlertMessage component

4982a22

Release note: None

ui: Enhance variable namings in tests

fd7139c

Release note: None

koorosh mentioned this pull request Feb 25, 2020

ui: add release notes signup to admin UI #43912

Closed

dhartunian approved these changes Feb 25, 2020

View reviewed changes

craig bot merged commit 7b4af8c into cockroachdb:master Feb 25, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ui: Release notes signup - Show Alert notification after signup#44856

ui: Release notes signup - Show Alert notification after signup#44856
craig[bot] merged 6 commits intocockroachdb:masterfrom
koorosh:ui-release-notes-signup__alert-notification

koorosh commented Feb 7, 2020 •

edited

Loading

Uh oh!

cockroach-teamcity commented Feb 7, 2020

Uh oh!

dhartunian Feb 24, 2020

Uh oh!

koorosh Feb 25, 2020

Uh oh!

dhartunian Feb 24, 2020

Uh oh!

koorosh Feb 25, 2020

Uh oh!

dhartunian Feb 24, 2020

Uh oh!

dhartunian Feb 24, 2020

Uh oh!

koorosh Feb 25, 2020

Uh oh!

dhartunian commented Feb 25, 2020

Uh oh!

craig bot commented Feb 25, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

koorosh commented Feb 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cockroach-teamcity commented Feb 7, 2020

Uh oh!

dhartunian Feb 24, 2020

Choose a reason for hiding this comment

Uh oh!

koorosh Feb 25, 2020

Choose a reason for hiding this comment

Uh oh!

dhartunian Feb 24, 2020

Choose a reason for hiding this comment

Uh oh!

koorosh Feb 25, 2020

Choose a reason for hiding this comment

Uh oh!

dhartunian Feb 24, 2020

Choose a reason for hiding this comment

Uh oh!

dhartunian Feb 24, 2020

Choose a reason for hiding this comment

Uh oh!

koorosh Feb 25, 2020

Choose a reason for hiding this comment

Uh oh!

dhartunian commented Feb 25, 2020

Uh oh!

craig bot commented Feb 25, 2020

Build succeeded

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

koorosh commented Feb 7, 2020 •

edited

Loading