Skip to content

Add borders between table rows #1284

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

Merged
merged 7 commits into from
Mar 24, 2025
Merged

Add borders between table rows #1284

merged 7 commits into from
Mar 24, 2025

Conversation

podliashanyk
Copy link
Contributor

@podliashanyk podliashanyk commented Mar 24, 2025

Scope and purpose

Fixes #1253

Contributor Checklist

Every pull request should have this checklist filled out, no matter how small it is.
More information about contributing to Argus can be found in the
Development docs.

  • Added a changelog fragment for towncrier
  • Linted/formatted the code with ruff and djLint, easiest by using pre-commit
  • The first line of the commit message continues the sentence "If applied, this commit will ...", starts with a capital letter, does not end with punctuation and is 50 characters or less long. See our how-to
  • If applicable: Created new issues if this PR does not fix the issue completely/there is further work to be done
  • If this results in changes in the UI: Added screenshots of the before and after
    • Before:
      Screenshot 2025-03-24 at 13 47 02
    • After:
      Screenshot 2025-03-24 at 13 55 29

@podliashanyk podliashanyk added the frontend Affects frontend label Mar 24, 2025
@podliashanyk podliashanyk requested review from a team March 24, 2025 12:47
@podliashanyk podliashanyk self-assigned this Mar 24, 2025
@github-project-automation github-project-automation bot moved this to 📋 Backlog in HTMXify Argus Mar 24, 2025
@podliashanyk podliashanyk moved this from 📋 Backlog to ❓ Ready for review in HTMXify Argus Mar 24, 2025
Copy link

github-actions bot commented Mar 24, 2025

Test results

   10 files  1 160 suites   38m 0s ⏱️
  564 tests   563 ✅  1 💤 0 ❌
5 640 runs  5 630 ✅ 10 💤 0 ❌

Results for commit 291c681.

♻️ This comment has been updated with latest results.

@codecov-commenter
Copy link

codecov-commenter commented Mar 24, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.74%. Comparing base (6b4d8de) to head (291c681).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1284   +/-   ##
=======================================
  Coverage   77.74%   77.74%           
=======================================
  Files         141      141           
  Lines        5662     5662           
=======================================
  Hits         4402     4402           
  Misses       1260     1260           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@podliashanyk podliashanyk force-pushed the add-borders-between-table-rows branch from 9c9e34e to cd838d8 Compare March 24, 2025 13:24
Copy link
Contributor

@hmpf hmpf left a comment

Choose a reason for hiding this comment

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

Looks fine to me

@github-project-automation github-project-automation bot moved this from ❓ Ready for review to 👍 Reviewer approved in HTMXify Argus Mar 24, 2025
@hmpf hmpf requested a review from a team March 24, 2025 13:28
@stveit
Copy link
Contributor

stveit commented Mar 24, 2025

Yes looks very similar to the lines shown in the old argus deployment

Question is how would this look on a big screen with the appropriate zoom. I think it would be good if we had another meeting with the service center guys where they test it themselves on the big screen and give feedback

@hmpf hmpf requested a review from a team March 24, 2025 13:35
@podliashanyk
Copy link
Contributor Author

Yes looks very similar to the lines shown in the old argus deployment

Question is how would this look on a big screen with the appropriate zoom. I think it would be good if we had another meeting with the service center guys where they test it themselves on the big screen and give feedback

Good point! I will merge it for now and make a new PR if they think it is too subtle on the big screen

@elfjes elfjes mentioned this pull request Mar 24, 2025
5 tasks
@podliashanyk
Copy link
Contributor Author

The users in the service center also would prefer continuous line for the borders, see #1285

@stveit stveit self-requested a review March 24, 2025 13:50
Copy link
Contributor

@stveit stveit left a comment

Choose a reason for hiding this comment

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

After testing on different themes: Looks good on Argus theme (against the light blue background), difficult to see on Light theme (white background) and pretty much invisible on dark theme, so this is something that must be configurable per theme

@podliashanyk
Copy link
Contributor Author

After testing on different themes: Looks good on Argus theme (against the light blue background), difficult to see on Light theme (white background) and pretty much invisible on dark theme, so this is something that must be configurable per theme

Good catch!

@podliashanyk podliashanyk moved this from 👍 Reviewer approved to 🏗 In progress in HTMXify Argus Mar 24, 2025
@hmpf hmpf merged commit c50386f into master Mar 24, 2025
17 checks passed
@hmpf hmpf deleted the add-borders-between-table-rows branch March 24, 2025 14:02
@github-project-automation github-project-automation bot moved this from 🏗 In progress to ✅ Done in HTMXify Argus Mar 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
frontend Affects frontend
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Add border between table rows
5 participants