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

Using material design for other tables as well #379

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

hector-vido
Copy link
Contributor

@hector-vido hector-vido commented Feb 17, 2025

What do you thing about adding the "default" material style for tables - used on https://prow.k8s.io/command-help - for other tables as well?

This let the appearance more appealing and modern and also give a visual feedback on what line we are with the mouse - very helpful.

This also shrink the font a little, but the final result is good in my opinion.

As a reference for the future, this is the version without style:
deck

This is the version with style and line feedback:
deck-material

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 17, 2025
@k8s-ci-robot
Copy link
Contributor

Hi @hector-vido. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Feb 17, 2025
@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Feb 17, 2025
Copy link

netlify bot commented Feb 17, 2025

Deploy Preview for k8s-prow ready!

Name Link
🔨 Latest commit 4232388
🔍 Latest deploy log https://app.netlify.com/sites/k8s-prow/deploys/67b3b4e8e744ac0008e009ba
😎 Deploy Preview https://deploy-preview-379--k8s-prow.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@@ -60,7 +60,7 @@
</aside>
<article>
<div class="table-container">
<table id="builds">
<table id="builds" class="mdl-data-table mdl-js-data-table mdl-shadow--2dp">
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe you could, similarly to @smg247 prepare deployment with proposed changes for us to see the difference.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@jmguzik jmguzik left a comment

Choose a reason for hiding this comment

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

/ok-to-test
/lgtm
It makes sense to have unified look. Please see attached deployment from @hector-vido. LGTM from me, but weather it will be merged or not, depends on taste :)

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 18, 2025
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 18, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: hector-vido, jmguzik
Once this PR has been reviewed and has the lgtm label, please assign smg247 for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@smg247
Copy link
Contributor

smg247 commented Feb 18, 2025

I am not a fan of the smaller font size, I think the rest of the look is good though. Can we try out a preview with the font size reverting to the original?

@k8s-ci-robot
Copy link
Contributor

@hector-vido: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-prow-integration 4232388 link true /test pull-prow-integration

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

@petr-muller
Copy link
Contributor

petr-muller commented Feb 18, 2025

I appreciate the effort of setting up an example instance! I really like the hover effect and let's find a way how we can get it working.

Some things we need to address (holding for these)
/hold

  1. Right-alignment of job names is the main breaker for me
  2. Deck lost responsivity. If I make the window narrow, it now forces me to scroll horizontally. Previously the table shrunk (some lines wrapped but that is a lesser evil than scrolling)

Some things that kinda bug me but I could live with them:

  1. The hover effect feels a little sluggish and I wonder if we could make it more "immediate"
  2. The font size, especially for the job name, feels too small, mainly disproportionate to the line height (and possibly to the size of the icons on the left) - I think if we made lines lower then it would work, in a kinda "compact" mode

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants