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

ui: Release notes signup - Email subscription form #44744

Merged

Conversation

koorosh
Copy link
Collaborator

@koorosh koorosh commented Feb 5, 2020

Resolves: #43912

  • Subscription form is responsible to accept user email, validate it and
    submit an appropriate action.
  • InputText component is styled according to design system.
  • Current changes are not connected to any view or page (in total isolation) and can be safely merged without affecting existing functionality.

@koorosh koorosh requested a review from a team February 5, 2020 10:59
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@koorosh koorosh changed the title ui: Email subscription form ui: Release notes signup - Email subscription form Feb 10, 2020
@koorosh koorosh force-pushed the ui-release-notes-signup__subscription-form branch from 1efd395 to eb9f109 Compare February 17, 2020 10:21
- Subscription form is responsible to accept user email, validate it and
submit an appropriate action.
- InputText component is styled according to design system.

Release note: None
- Button component extended with styles for "primary" type
- InputText component is refactored to be as fully controlled
component.

Release note: None
@koorosh koorosh force-pushed the ui-release-notes-signup__subscription-form branch from eb9f109 to 7e5af42 Compare February 17, 2020 10:37
- Display validation message for Input component with
absolute position to prevent layout to be expanded.
- Change validation message for invalid email address.

Release note: None
@koorosh koorosh mentioned this pull request Feb 18, 2020
2 tasks
Add enhancements for InputText component:
- validation of input is performed during text change
- set fixed width for email input

Release note: None
Copy link
Collaborator

@dhartunian dhartunian left a comment

Choose a reason for hiding this comment

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

@koorosh just a small nit

type={"primary"}
onClick={this.handleSubmit}
disabled={!canSubmit}
className="email-subscription-form__submit-button"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why does this button need a custom className? If the text within is always there, it doesn't seem like the min-width is necessary.

Copy link
Collaborator Author

@koorosh koorosh Feb 25, 2020

Choose a reason for hiding this comment

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

Actually it requires min-width to avoid words wrapping in two lines (when window is resized to small width for example).

});
});

describe("when invalid email", () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Great test coverage on the functionality!

@dhartunian
Copy link
Collaborator

bors r+

@craig
Copy link
Contributor

craig bot commented Feb 25, 2020

Build succeeded

@craig craig bot merged commit bef2eb2 into cockroachdb:master Feb 25, 2020
@koorosh koorosh deleted the ui-release-notes-signup__subscription-form branch February 25, 2020 10:20
craig bot pushed a commit that referenced this pull request Feb 28, 2020
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>
craig bot pushed a commit that referenced this pull request Feb 28, 2020
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>
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.

ui: add release notes signup to admin UI
3 participants