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

[dashboard] Unify and standardize InfoBox and AlertBox components #4020

Merged
merged 1 commit into from
Apr 22, 2021

Conversation

jankeromnes
Copy link
Contributor

@jankeromnes jankeromnes commented Apr 20, 2021

Fixes #3996

TODO:

@jankeromnes jankeromnes force-pushed the jx/info-alert-box branch 4 times, most recently from 6df5fb7 to 169b7e1 Compare April 21, 2021 07:16
@jankeromnes jankeromnes marked this pull request as ready for review April 21, 2021 07:29
@jankeromnes
Copy link
Contributor Author

jankeromnes commented Apr 21, 2021

@gtsiolis Could you please take a look when you have time?

Priority: 🔅 Low.

@jankeromnes jankeromnes requested a review from gtsiolis April 21, 2021 07:50
@gtsiolis
Copy link
Contributor

gtsiolis commented Apr 21, 2021

/werft run

👍 started the job as gitpod-build-jx-info-alert-box.6

Copy link
Contributor

@gtsiolis gtsiolis left a comment

Choose a reason for hiding this comment

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

Thanks for componetizing this element @jankeromnes! 💯

Added a few more unrelated comments to colors. Feel free to leave these out of the scope of changes here. We're still missing some color correction in modals but I think we can tackle these separately once we componetize the modals.

</div>
<InfoBox className="w-2/3 mt-14 mx-auto">
If you are interested in purchasing a plan for a team, purchase a Team plan with one centralized billing. <a className="underline" href="https://www.gitpod.io/docs/teams/">Learn more</a>
</InfoBox>
{!!confirmUpgradeToPlan && <Modal visible={true} onClose={() => setConfirmUpgradeToPlan(undefined)}>
<h3>Upgrade to {confirmUpgradeToPlan.name}</h3>
<div className="border-t border-b border-gray-200 dark:border-gray-800 mt-4 -mx-6 px-6 py-2">
<p className="mt-1 mb-4 text-base">You are about to upgrade to {confirmUpgradeToPlan.name}.</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

issue: We're going to need dark:text-gray-500 for all paragraph text within modals. 🌙

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! Many other modals already have text-gray-500 for both light and dark. Will align here, but leave additional componentization of Modals as follow-up.

components/dashboard/src/components/AlertBox.tsx Outdated Show resolved Hide resolved
components/dashboard/src/components/AlertBox.tsx Outdated Show resolved Hide resolved
@gtsiolis
Copy link
Contributor

gtsiolis commented Apr 21, 2021

Also, thank you for adding all these screenshots! These are really helpful during reviewing visual changes.

Tip: I usually find the table element in markdown extremely useful to represent such information. For example, using for BEFORE / AFTER screenshots or adding some structure to unrelated screenshots. Feel free to try this or keep adding your awesome contributions without any table!💡

# EXAMPLE A

| BEFORE | AFTER |
|-|-|
| ![image](...) | ![image](...) | 

# EXAMPLE B

| Screenshot A | Screenshot B | Screenshot C |
|-|-|-|
| ![image](...) | ![image](...) | ![image](...) | 

@jankeromnes
Copy link
Contributor Author

jankeromnes commented Apr 22, 2021

Many thanks for the great review! 💯

Also, thank you for adding all these screenshots! These are really helpful during reviewing visual changes.

Sure! Glad they're helpful. ✨

Tip: I usually find the table element in markdown extremely useful to represent such information. For example, using for BEFORE / AFTER screenshots or adding some structure to unrelated screenshots. Feel free to try this or keep adding your awesome contributions without any table!💡

Haha, great tip! I agree, although I typically go old-school with a bunch of:

<table><tr><td>
<img/>
</td><td>
<img/>
</td></tr></table>

(😅)

@jankeromnes
Copy link
Contributor Author

jankeromnes commented Apr 22, 2021

Also, what should we do about the "Not yet unified AlertBoxes" in #4020 (comment) ?

They currently have a different visual style, which is why I haven't unified them yet. Should they stay different for now?

@jankeromnes jankeromnes merged commit b4d64c3 into main Apr 22, 2021
@jankeromnes jankeromnes deleted the jx/info-alert-box branch April 22, 2021 08:20
@gtsiolis
Copy link
Contributor

Go <table>!

They currently have a different visual style ...

Yes, the ones within the modals should eventually use the same visual style. The garbage collection alert could use a larger component variant.

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.

[dashboard] Unify Alert components & fix for dark theme
2 participants