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

Use Jenkins design library tables and colors #424

Merged
merged 4 commits into from
Jan 13, 2023

Conversation

cronik
Copy link
Contributor

@cronik cronik commented Dec 3, 2022

Switch to Jenkins design library table classes. Remove data-tables-api dependency since sorting is handled by jenkins tables by default.

Before:
Screenshot 2022-12-03 at 6 26 39 PM

After:
Screenshot 2022-12-03 at 6 26 10 PM

See JENKINS-XXXXX.

See #XXXXX

Testing done

Proposed upgrade guidelines

N/A

Localizations

  • English
  • German
  • French
  • Slovak
  • Czech
  • ...

Submitter checklist

  • The Jira / Github issue, if it exists, is well-described.
  • The changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developers, depending on the change) and are in the imperative mood (see examples).
    • The changelog generator for plugins uses the pull request title as the changelog entry.
    • Fill in the Proposed upgrade guidelines section only if there are breaking changes or changes that may require extra steps from users during the upgrade.
  • There is automated testing or an explanation that explains why this change has no tests.
  • New public functions for internal use only are annotated with @NoExternalUse. In case it is used by non java code the Used by {@code <panel>.jelly} Javadocs are annotated.
  • New or substantially changed JavaScript is not defined inline and does not call eval to ease the future introduction of Content Security Policy (CSP) directives (see documentation).
  • For dependency updates, there are links to external changelogs and, if possible, full differentials.
  • For new APIs and extension points, there is a link to at least one consumer.
  • Any localizations are transferred to *.properties files.
  • Changes in the interface are documented also as examples.

Maintainer checklist

Before the changes are marked as ready-for-merge:

  • There is at least one (1) approval for the pull request and no outstanding requests for change.
  • Conversations in the pull request are over, or it is explicit that a reviewer is not blocking the change.
  • Changelog entries in the pull request title and/or Proposed changelog entries are accurate, human-readable, and in the imperative mood.
  • Proper changelog labels are set so that the changelog can be generated automatically. See also release-drafter-labels.
  • If the change needs additional upgrade steps from users, the upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the pull request title (see example).
  • java code changes are tested by automated test.

@cronik cronik requested a review from a team as a code owner December 3, 2022 23:32
@mPokornyETM
Copy link
Contributor

I am not happy with this change, because data-tables provides more features then jenkins-table. Ex. filtering, paging ...
I am working on the fix for sorting currently. See #415
But I like the icon change as well. So can you pls try to change it in the data tables (also the things wit colors?) and I will complete ma changes. So we get good compromise.

@cronik
Copy link
Contributor Author

cronik commented Dec 16, 2022

Added data-tables back.

Screenshot 2022-12-16 at 3 10 55 PM

Moved the lockable-resources.js script import into the <l:main-panel> block, I noticed the rendered html included the script 2x in the previous location.

@mPokornyETM
Copy link
Contributor

I have fixed it in #415 When you want, you can review the changes there. Also you can play with colors there.

@mPokornyETM
Copy link
Contributor

Please verify current master, if I have implemented all your proposals from here. Thx

@cronik
Copy link
Contributor Author

cronik commented Dec 21, 2022

Rebased with master and applied Jenkins Design Library styles.

Fixed a NPE that occurred while testing.

Screenshot 2022-12-20 at 9 23 57 PM

@mPokornyETM
Copy link
Contributor

mPokornyETM commented Dec 21, 2022

Hm this is very interesting. I look here 3 times in last 24 hours to make some decisions. And I am still not sure.

Please read this comment, there is no criticism. I am just thinking too loud. 😉

Why you prefer jenkins tables look instead of bootstrap solution? When you has many and I mean more then 100 resources with big descriptions and all of them contains many labels it looks much better, then jenkins design.

Everything else here LGTM. I am just not happy with the design of jenkins tables as well. Think about table in /computer page. 😬 and as I see, also in this case are necessary stylesheet hack to make it more pretty. I think, that's the reason why so many plugins use the datatables plugin.
Also the actions buttons does not look nice to me. So I am not sure what is better 🤔 , because this is about my personal user feeling.

Try to use a example with more resources and more data and then we can discuss is later. Thx.

Apropos later. Probably I am online only on my cell phone for next 2 weeks. Therefore it might take a while until you get a answer.

@mPokornyETM
Copy link
Contributor

And please fill pr description in proper way. Thx

@cronik
Copy link
Contributor Author

cronik commented Dec 23, 2022

My goal with this PR was to use the Jenkins Design system to make the look consistent with the rest of Jenkins, not that I necessarily prefer one or the other. Using the standard design system also has the benefit that it works better with themes. Notice the "odd" row text in the dark theme is not readable and the buttons are not themed in the examples below.

Here are 2 screenshots comparing the jenkins design tables versus the default data tables style. The total height of the jenkins table version is ~530px more due to the increased padding. Functionally they are they same (i.e. sorting, pagination, search).

If the issue is with the Jenkins design library table styling I can submit a separate PR with just the NPE and doc fixes.

jenkins-tables
datatables
jenkins-tables-dark
datatables-dark

Switch to Jenkins design library table classes.
Copy link
Contributor

@mPokornyETM mPokornyETM left a comment

Choose a reason for hiding this comment

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

LGTM

@mPokornyETM
Copy link
Contributor

This PR fix the dark mode only partly. I thing it will be better, when somebody fix it in data-table-plugin. See also discussions in #415

image
image

Copy link
Contributor

@mPokornyETM mPokornyETM left a comment

Choose a reason for hiding this comment

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

LGTM

@mPokornyETM mPokornyETM added enhancement ui Features that may impact UI, pages made by the plugin or external UIs (BO, legacy, etc.) labels Jan 13, 2023
@mPokornyETM mPokornyETM enabled auto-merge (squash) January 13, 2023 21:41
@mPokornyETM mPokornyETM merged commit 470ba94 into jenkinsci:master Jan 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement ui Features that may impact UI, pages made by the plugin or external UIs (BO, legacy, etc.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants