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

Make links in tables more clearly clickable #1899

Merged
merged 9 commits into from
Jan 29, 2024
Merged

Conversation

benjaminleonard
Copy link
Contributor

@benjaminleonard benjaminleonard commented Jan 22, 2024

Follow up from #1886 and feedback from the team.

  • Whole row hover state (helps distinguish between rows)
  • Link cells are now the same style universally
  • Fixed disk cell that was styled differently

CleanShot 2024-01-22 at 15 56 19

I'd would have liked to have the first cell be text-default but unsure what to do with the hover state in that instance since we don't have a colour brighter than that.

Copy link

vercel bot commented Jan 22, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
console ✅ Ready (Inspect) Visit Preview Jan 29, 2024 4:01pm

@benjaminleonard benjaminleonard marked this pull request as draft January 22, 2024 15:59
@zephraph
Copy link
Contributor

I'm noticing in the gif that the more action button gets a slightly different color than the other row cells on hover. Could we make that consistent? I like the idea of the row being highlighted on hover, but it's also nice if when a cell is clickable it's hover color is differentiable. The underlines are nice too as a persistent reminder.

Comment on lines +47 to +51
const instanceLinkCell = linkCell((instanceName) =>
pb.instancePage({ ...projectSelector, instance: instanceName })
)

return instance ? instanceLinkCell({ value: instance.name }) : null
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, happy to see the reuse of linkcell here

className="flex h-full w-full items-center text-sans-semi-md text-default hover:underline"
className="flex h-full w-full items-center underline text-sans-semi-md text-secondary hover:text-default"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is hover:bg-raise to distracting here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was kinda thinking the same

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"bg-hover" is too strong, and my concern was that "bg-raise" was inconsistent.

My rationale for more actions and the link cell having a different treatment is partly the distinction between a button and a link but that's perhaps a weak argument.

I'll collect our options and drop them in here and we can decide which is working the best.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, my preference would be to treat clickable things consistently, regardless of what kind of clickable thing they are. I feel like bg-raise does a good job of calling extra attention which (when combined with a pointer) is a doubly helpful indication.

@benjaminleonard
Copy link
Contributor Author

Few options:

  1. Both links and more actions button have filled contained
    CleanShot 2024-01-29 at 12 53 04

  2. bg-raise on hover across entire cell
    CleanShot 2024-01-29 at 12 54 29

  3. Same but icon has it's own fill for emphasis
    CleanShot 2024-01-29 at 12 57 02

@zephraph @david-crespo @paryhin

@david-crespo
Copy link
Collaborator

david-crespo commented Jan 29, 2024

None are bad but I think 2 is most natural. Like Eugene said, the permanent underline on links is most accessible, even though it feels a little noisy.

@benjaminleonard
Copy link
Contributor Author

None are bad but I think 2 is most natural. Like Eugene said, the permanent underline on links is most accessible, even though it feels a little noisy.

Agreed. On the underline. text-decoration-color could be useful to reduce some of that noise:

CleanShot 2024-01-29 at 14 51 45

Copy link
Collaborator

@david-crespo david-crespo left a comment

Choose a reason for hiding this comment

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

Tried it in the preview and it's honestly fantastic. Didn't realize what a difference it would make.

@benjaminleonard
Copy link
Contributor Author

Perfect, just spotted a little stroke overlap I need to fix then will merge.

@benjaminleonard benjaminleonard enabled auto-merge (squash) January 29, 2024 16:07
@benjaminleonard benjaminleonard merged commit 1fb746f into main Jan 29, 2024
8 checks passed
@benjaminleonard benjaminleonard deleted the link-cell-underline branch January 29, 2024 16:09
david-crespo added a commit to oxidecomputer/omicron that referenced this pull request Jan 31, 2024
oxidecomputer/console@b9013a3...1a4f5d8

* [1a4f5d81](oxidecomputer/console@1a4f5d81) oxidecomputer/console#1910
* [34596e33](oxidecomputer/console@34596e33) oxidecomputer/console#1928
* [ff7cdb28](oxidecomputer/console@ff7cdb28) oxidecomputer/console#1926
* [efa39789](oxidecomputer/console@efa39789) oxidecomputer/console#1867
* [4bfadc02](oxidecomputer/console@4bfadc02) oxidecomputer/console#1927
* [695d3671](oxidecomputer/console@695d3671) oxidecomputer/console#1925
* [30070292](oxidecomputer/console@30070292) better path filter for local files in msw handler
* [5cf4339c](oxidecomputer/console@5cf4339c) oxidecomputer/console#1916
* [a26f7c1e](oxidecomputer/console@a26f7c1e) oxidecomputer/console#1922
* [231b93ed](oxidecomputer/console@231b93ed) oxidecomputer/console#1923
* [c8364638](oxidecomputer/console@c8364638) better msw warning filter so we don't get warning noise in console
* [764f7310](oxidecomputer/console@764f7310) oxidecomputer/console#1908
* [945619eb](oxidecomputer/console@945619eb) oxidecomputer/console#1921
* [e2d82a4c](oxidecomputer/console@e2d82a4c) oxidecomputer/console#1887
* [d6a67bd5](oxidecomputer/console@d6a67bd5) oxidecomputer/console#1918
* [1fb746f4](oxidecomputer/console@1fb746f4) oxidecomputer/console#1899
* [ca7c85de](oxidecomputer/console@ca7c85de) oxidecomputer/console#1917
* [28598e1d](oxidecomputer/console@28598e1d) oxidecomputer/console#1914
* [34eb478d](oxidecomputer/console@34eb478d) oxidecomputer/console#1912
* [4d693088](oxidecomputer/console@4d693088) bump vite-plugin-html to stop getting deprecation warning
* [d5c39549](oxidecomputer/console@d5c39549) oxidecomputer/console#1909
* [7c6f53db](oxidecomputer/console@7c6f53db) oxidecomputer/console#1854
david-crespo added a commit to oxidecomputer/omicron that referenced this pull request Jan 31, 2024
oxidecomputer/console@b9013a3...1a4f5d8

* [1a4f5d81](oxidecomputer/console@1a4f5d81)
oxidecomputer/console#1910
* [34596e33](oxidecomputer/console@34596e33)
oxidecomputer/console#1928
* [ff7cdb28](oxidecomputer/console@ff7cdb28)
oxidecomputer/console#1926
* [efa39789](oxidecomputer/console@efa39789)
oxidecomputer/console#1867
* [4bfadc02](oxidecomputer/console@4bfadc02)
oxidecomputer/console#1927
* [695d3671](oxidecomputer/console@695d3671)
oxidecomputer/console#1925
* [30070292](oxidecomputer/console@30070292)
better path filter for local files in msw handler
* [5cf4339c](oxidecomputer/console@5cf4339c)
oxidecomputer/console#1916
* [a26f7c1e](oxidecomputer/console@a26f7c1e)
oxidecomputer/console#1922
* [231b93ed](oxidecomputer/console@231b93ed)
oxidecomputer/console#1923
* [c8364638](oxidecomputer/console@c8364638)
better msw warning filter so we don't get warning noise in console
* [764f7310](oxidecomputer/console@764f7310)
oxidecomputer/console#1908
* [945619eb](oxidecomputer/console@945619eb)
oxidecomputer/console#1921
* [e2d82a4c](oxidecomputer/console@e2d82a4c)
oxidecomputer/console#1887
* [d6a67bd5](oxidecomputer/console@d6a67bd5)
oxidecomputer/console#1918
* [1fb746f4](oxidecomputer/console@1fb746f4)
oxidecomputer/console#1899
* [ca7c85de](oxidecomputer/console@ca7c85de)
oxidecomputer/console#1917
* [28598e1d](oxidecomputer/console@28598e1d)
oxidecomputer/console#1914
* [34eb478d](oxidecomputer/console@34eb478d)
oxidecomputer/console#1912
* [4d693088](oxidecomputer/console@4d693088)
bump vite-plugin-html to stop getting deprecation warning
* [d5c39549](oxidecomputer/console@d5c39549)
oxidecomputer/console#1909
* [7c6f53db](oxidecomputer/console@7c6f53db)
oxidecomputer/console#1854
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.

3 participants