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] add button.gp-link CSS class #9641

Merged
merged 1 commit into from
May 11, 2022

Conversation

trumbitta
Copy link
Contributor

Description

Adds a .gp-link Tailwind class to make buttons mimic links and avoid using <a href="javascript:void(0)" ...>

To be used for anchor tags that should've been buttons because they don't really bring the user to any new location like an anchor tag usually does.

Just like #9269, this is about #3841 and its purpose is to simplify to the max #8995.

Related Issue(s)

Re #3841

How to test

  • launch the dashboard app
  • the two updated links which are now buttons should look and work just like before

Release Notes

- [dashboard]: Added a `.gp-link` Tailwind class to make buttons mimic anchor tags

@laushinka
Copy link
Contributor

Thanks, @trumbitta! Let me loop in @gtsiolis here for this one :)

@gtsiolis
Copy link
Contributor

gtsiolis commented Apr 29, 2022

Looking at this now! 👀

/werft run

👍 started the job as gitpod-build-link-button-fork.0
(with .werft/ from main)

@gtsiolis
Copy link
Contributor

gtsiolis commented Apr 29, 2022

/werft run

👍 started the job as gitpod-build-link-button-fork.1
(with .werft/ from main)

@gtsiolis
Copy link
Contributor

gtsiolis commented May 3, 2022

@trumbitta could you rebase this to main branch to fix the failing build?

@gtsiolis
Copy link
Contributor

gtsiolis commented May 3, 2022

/werft run

👍 started the job as gitpod-build-link-button-fork.2
(with .werft/ from main)

a.gp-link {
@apply text-blue-500 hover:text-blue-600 dark:text-blue-400 dark:hover:text-blue-500;
button.gp-link {
@apply bg-transparent hover: bg-transparent p-0 rounded-none;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

wait a minute... where does this extra white-space between hover: and bg-transparent come from 🤔

fixing it now (also in the next selector)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gtsiolis should be fixed now. Sorry for the hiccup, I don't really know how it happened 😮

To be used for anchor tags that should've been buttons because
they don't really bring the user to any new location.
@gtsiolis
Copy link
Contributor

gtsiolis commented May 6, 2022

/werft run

👍 started the job as gitpod-build-link-button-fork.3
(with .werft/ from main)

@gtsiolis
Copy link
Contributor

gtsiolis commented May 6, 2022

/werft run

👍 started the job as gitpod-build-link-button-fork.4
(with .werft/ from main)

@gtsiolis
Copy link
Contributor

gtsiolis commented May 6, 2022

/werft run 🎲

👍 started the job as gitpod-build-link-button-fork.5
(with .werft/ from main)

@gtsiolis
Copy link
Contributor

gtsiolis commented May 6, 2022

/werft run 🎲🎲

👍 started the job as gitpod-build-link-button-fork.6
(with .werft/ from main)

@trumbitta
Copy link
Contributor Author

Is werft having a bad day?

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 opening this PR, your patience, and caring about accessibility with replacing these links with buttons, @trumbitta! 💯

To add some background context here, the introduction of the gp-link class in #5112 initially aimed to abstract the style used for help links. 🔗

However, the long-term goal is to have a button component variant styled as a link for actions like the ones changed in this PR, instead of maintaining CSS styles in the index.css. 💡

Ideally, we'd like to introduce a button component for what we need here but I don't want to block or close this pull request since we're going to update and replace the pages affected here soon. 🚀

UX LGTM!

@laushinka could you take another look and merge this or pass this to someone on the team since this requires also an approval from the WebApp team?

@@ -493,13 +493,12 @@ export default function NewProject() {
<div>
<div className="text-gray-500 text-center w-96 mx-8">
Repository not found?{" "}
<a
href="javascript:void(0)"
Copy link
Contributor

Choose a reason for hiding this comment

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

praise: Nice!

@@ -493,13 +493,12 @@ export default function NewProject() {
<div>
<div className="text-gray-500 text-center w-96 mx-8">
Repository not found?{" "}
<a
href="javascript:void(0)"
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion(non-blocking): We have a few more actions in /plans masked as links that could benefit from this. However, I wouldn't block this from merging it as we're going to update and replace plans entirly soon.

<a
className="text-blue-light leading-6"
href="javascript:void(0)"
onClick={() => confirmCancelDowngrade()}
>
Cancel
</a>

<a
className="text-blue-light leading-6"
href="javascript:void(0)"
onClick={() => confirmCancelDowngrade()}
>
Cancel
</a>

...

@@ -368,9 +368,9 @@ export default function NewProject() {
<p className="text-gray-500 text-center text-base mt-12">
{loaded && noReposAvailable ? "Select account on " : "Select a Git repository on "}
<b>{selectedProviderHost}</b> (
<a className="gp-link cursor-pointer" onClick={() => setShowGitProviders(true)}>
<button className="gp-link cursor-pointer" onClick={() => setShowGitProviders(true)}>
change
Copy link
Contributor

Choose a reason for hiding this comment

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

issue(non-blocking): This will slightly affect how the link style is rendered but that's minor for now.

BEFORE AFTER
link-before link-after

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 🏅 !

I guess it stems from text-sm in the Tailwind styles applied to the button element while a seems to just inherit as usual.

The upcoming button component should take it into account. If you need a volunteer for working on the component, you know where to find me 😊

@gtsiolis gtsiolis requested review from laushinka and removed request for laushinka May 6, 2022 14:47
@laushinka
Copy link
Contributor

laushinka commented May 11, 2022

@trumbitta I need to stop apologizing to you and just review faster! This fell through the cracks of my reviewing cycle 🙇🏻‍♀️

LGTM

@roboquat roboquat merged commit 2f988c9 into gitpod-io:main May 11, 2022
@trumbitta
Copy link
Contributor Author

trumbitta commented May 11, 2022

@trumbitta I need to stop apologizing to you and just review faster! This fell through the cracks of my reviewing cycle 🙇🏻‍♀️

LGTM

🙏 @laushinka I'm yet to find anything you should apologize for. If anything, I owe you because sometimes I just find bits of your code in the dashboard codebase that are so good I want to scream.

@laushinka
Copy link
Contributor

laushinka commented May 11, 2022

@trumbitta Omg whattt you just made my day. If I ever feel down I will look at this comment 🧡

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants