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

Replace AlertBox with Alert component #10199

Merged
merged 1 commit into from
Aug 5, 2022
Merged

Replace AlertBox with Alert component #10199

merged 1 commit into from
Aug 5, 2022

Conversation

KevSlashNull
Copy link
Contributor

@KevSlashNull KevSlashNull commented May 23, 2022 •
edited by gtsiolis
Loading

Description

This PR replaces the old AlertBox with the new Alert component (type warning) across the dashboard. This change is a part of the overall effort to migrate to the new Alert component in #9411.

Where Before After
Integration activation image image
Project variables image image
Upgrade modal image image
Team plan pricing image image
Confirmation modal, warning text image image

Related Issue(s)

Fix #9411

How to test

Integration activation

  1. Go to Settings > Integrations
  2. Click on New Integration
  3. Fill it with dummy data (e.g. Provider Host Name: example.codingpa.ws, Application ID: test, Secret: test)
  4. Click on Activate integration
  5. Close the popup
  6. See the changed alert

Project variables

  1. Go to a project (or create one)
  2. Go to Settings > Variables in the project
  3. Click on New variable
  4. See the changed alert

Upgrade modal

  1. Go to Settings > Plans
  2. Click on a plan (You need to not be on that plan)
  3. See the changed alert in the modal

Team plan pricing

  1. Go to Settings > Team Plans
  2. Click on Create Team Plan
  3. See the changed alert

Confirmation modal

  1. Go to a team (or create one)
  2. In the team, go to Settings
  3. Click on Delete Team
  4. See the changed alert in the modal

Release Notes

Migrate AlertBox component instances to Alert component

Documentation

Werft options:

  • /werft with-preview

@KevSlashNull KevSlashNull requested a review from a team May 23, 2022 21:14
@gtsiolis gtsiolis self-assigned this May 23, 2022
@gtsiolis gtsiolis self-requested a review May 24, 2022 05:56
gtsiolis
gtsiolis approved these changes May 24, 2022
View reviewed changes
Copy link
Contributor

@gtsiolis gtsiolis left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks @KevSlashNull for contributing, the detailed write-up on how to test this, and the before and after screenshots!

  1. Re-posting from Fix link to Code of Conduct in README #10170 (comment), let me loop in @meysholdt to reach out about the CLA. 🏓
  2. UX and code changes look good! ✔️
  3. Opened a new follow up issue for the InfoBox component migration in Migrate InfoBox components to the new Alert component #10205, see Migrate AlertBox components to the new Alert component #9411 (comment). ➿
  4. Let me also spin up a preview environment for this PR below. 🚀

@gtsiolis
Copy link
Contributor

gtsiolis commented May 24, 2022 •
edited by werft-gitpod-dev-com bot
Loading

/werft run

👍 started the job as gitpod-build-9411-replace-alertbox-with-alert-component-fork.0
(with .werft/ from main)

@meysholdt
Copy link
Member

hi @KevSlashNull! To send you our CLA, can you please send me an email to moritz@gitpod.io with your full name? I will then send the CLA via DocuSign to your email address.

@KevSlashNull
Copy link
Contributor Author

KevSlashNull commented May 24, 2022 •
edited by werft-gitpod-dev-com bot
Loading

Thanks @meysholdt, I’ve sent you an email. 😄

@stale
Copy link

stale bot commented Jun 10, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the meta: stale This issue/PR is stale and will be closed soon label Jun 10, 2022
@KevSlashNull
Copy link
Contributor Author

KevSlashNull commented Jun 17, 2022 •
edited by werft-gitpod-dev-com bot
Loading

Closing this PR as I haven’t signed the CLA yet (blocked by #7864) to reduce the signal-noise-ratio. Once that is resolved, I’ll gladly pick this up again.

cc @gtsiolis

@gtsiolis
Copy link
Contributor

gtsiolis commented Jun 27, 2022 •
edited
Loading

Let me reopen this, @KevSlashNull! The issue in #7864 should not be a blocker for signing the CLA and merging this. I've pinged the team internally.

Have you signed the CLA that @meysholdt mentioned in #10199 (comment)?

@gtsiolis gtsiolis reopened this Jun 27, 2022
@stale stale bot removed the meta: stale This issue/PR is stale and will be closed soon label Jun 27, 2022
@KevSlashNull
Copy link
Contributor Author

KevSlashNull commented Jun 27, 2022 via email •
edited by werft-gitpod-dev-com bot
Loading

@stale
Copy link

stale bot commented Jul 10, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the meta: stale This issue/PR is stale and will be closed soon label Jul 10, 2022
@gtsiolis gtsiolis removed the meta: stale This issue/PR is stale and will be closed soon label Jul 11, 2022
@meysholdt
Copy link
Member

hi @KevSlashNull, we've updated our CLA process and the controversial language of #7864 is no longer part of our CLA. Can you please sign our CLA via this DocuSign form? If there are any questions, you can reach me via moritz@gitpod.io.

@KevSlashNull
Copy link
Contributor Author

KevSlashNull commented Aug 4, 2022 •
edited by werft-gitpod-dev-com bot
Loading

@meysholdt, cool, thank you! 😄 I signed the CLA. Once it’s signed by you as well, I’ll push a commit to fix the conflicts to get this PR moving.

@meysholdt meysholdt added cla: accepted ✔️ and removed do-not-merge/cla-pending CLA has not been signed labels Aug 4, 2022
@meysholdt
Copy link
Member

hi @KevSlashNull, thank you for signing! I have signed it as well.

@gtsiolis
Copy link
Contributor

gtsiolis commented Aug 4, 2022 •
edited by werft-gitpod-dev-com bot
Loading

Thanks @meysholdt! @KevSlashNull could you resolve conflicts and rebase against main? 🏓

@KevSlashNull
Replace AlertBox with Alert component
888f858 
This replaces the old AlertBox with the new Alert (type `warning`) in
the dashboard.
@KevSlashNull KevSlashNull force-pushed the 9411-Replace-AlertBox-with-Alert-component branch from b0d4b5a to 888f858 Compare August 4, 2022 18:59
@KevSlashNull
Copy link
Contributor Author

KevSlashNull commented Aug 4, 2022 •
edited by werft-gitpod-dev-com bot
Loading

Thanks @meysholdt!

@gtsiolis, I’ve rebased and fixed the conflicts. Back to you! 🏓

@KevSlashNull KevSlashNull removed their assignment Aug 4, 2022
@gtsiolis
Copy link
Contributor

gtsiolis commented Aug 4, 2022 •
edited by werft-gitpod-dev-com bot
Loading

/werft run

👍 started the job as gitpod-build-9411-replace-alertbox-with-alert-component-fork.1
(with .werft/ from main)

@werft-gitpod-dev-com
Copy link

started the job as gitpod-build-9411-replace-alertbox-with-alert-component.0 because the annotations in the pull request description changed
(with .werft/ from main)

@gtsiolis
Copy link
Contributor

gtsiolis commented Aug 4, 2022 •
edited by werft-gitpod-dev-com bot
Loading

/werft with-preview

👎 unknown command: with-preview
Use /werft help to list the available commands

@gtsiolis
Copy link
Contributor

gtsiolis commented Aug 4, 2022 •
edited by werft-gitpod-dev-com bot
Loading

/werft run with-preview

👍 started the job as gitpod-build-9411-replace-alertbox-with-alert-component-fork.2
(with .werft/ from main)

gtsiolis
gtsiolis approved these changes Aug 4, 2022
View reviewed changes
Copy link
Contributor

@gtsiolis gtsiolis left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Approving once more after running the preview environment. You should be able to access the preview environment for the next couple of hours before it times out.

UX and code changes, LGTM. ✔️

Thanks again @KevSlashNull for contributing and for your patience while our CLA signing process was under the microscope. 🍊

This will need one more approval from someone from the @gitpod-io/engineering-webapp—Explicitly pinging @gitpod-io/engineering-webapp for visibility.

@gtsiolis gtsiolis added team: webapp Issue belongs to the WebApp team component: dashboard type: improvement Improves an existing feature or existing code labels Aug 4, 2022
@roboquat roboquat merged commit 93335ab into gitpod-io:main Aug 5, 2022
@gtsiolis
Copy link
Contributor

gtsiolis commented Aug 5, 2022 •
edited by werft-gitpod-dev-com bot
Loading

Thanks for taking a look @andrew-farries! 🏀

@KevSlashNull
Copy link
Contributor Author

Thanks @gtsiolis and @andrew-farries! 🙌

@KevSlashNull KevSlashNull deleted the 9411-Replace-AlertBox-with-Alert-component branch August 5, 2022 07:47
@roboquat roboquat added deployed: webapp Meta team change is running in production deployed Change is completely running in production labels Aug 5, 2022
@tnir tnir mentioned this pull request Sep 24, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gtsiolis gtsiolis gtsiolis approved these changes

@andrew-farries andrew-farries andrew-farries approved these changes

Assignees

@gtsiolis gtsiolis

Labels
cla: accepted ✔️ community-contribution component: dashboard deployed: webapp Meta team change is running in production deployed Change is completely running in production release-note size/M team: webapp Issue belongs to the WebApp team type: improvement Improves an existing feature or existing code
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Migrate AlertBox components to the new Alert component
5 participants
@KevSlashNull @gtsiolis @meysholdt @andrew-farries @roboquat