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 new table icons in Workbench column headers #5176

Merged
merged 9 commits into from
Nov 21, 2024
Merged

Conversation

sharadsw
Copy link
Contributor

@sharadsw sharadsw commented Aug 5, 2024

Fixes #2864

Checklist

  • Self-review the PR after opening it to make sure the changes look good
    and self-explanatory (or properly documented)
  • Add automated tests
  • Add relevant issue to release milestone

Testing instructions

  • Create a workbench dataset
  • Verify new icons are being used in the column headers
  • Verify icons work fine for new geo tables (eg: COG, COGType, age tables etc.)
  • Verify table icons look the same as before everywhere else (Data Entry, Schema Config etc.)
  • Verify icons look fine after changing font size or other text options

@sharadsw sharadsw changed the base branch from production to issue-5169 August 5, 2024 14:53
Base automatically changed from issue-5169 to production August 5, 2024 19:53
Copy link
Member

@maxpatiiuk maxpatiiuk left a comment

Choose a reason for hiding this comment

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

looks good!

somewhat related:
since you refactored the workbench, could you double-check if some of these legacy icons can now be removed?

arrowsExpand: `<svg class="${iconClassName}" aria-hidden="true" fill="currentColor" viewBox="0 0 20 20" xmlns="http://www.w3.org/2000/svg"><path fill-rule="evenodd" d="M3 4a1 1 0 011-1h4a1 1 0 010 2H6.414l2.293 2.293a1 1 0 11-1.414 1.414L5 6.414V8a1 1 0 01-2 0V4zm9 1a1 1 0 010-2h4a1 1 0 011 1v4a1 1 0 01-2 0V6.414l-2.293 2.293a1 1 0 11-1.414-1.414L13.586 5H12zm-9 7a1 1 0 012 0v1.586l2.293-2.293a1 1 0 111.414 1.414L6.414 15H8a1 1 0 010 2H4a1 1 0 01-1-1v-4zm13-1a1 1 0 011 1v4a1 1 0 01-1 1h-4a1 1 0 010-2h1.586l-2.293-2.293a1 1 0 111.414-1.414L15 13.586V12a1 1 0 011-1z" clip-rule="evenodd"></path></svg>`,
ban: `<svg class="${iconClassName}" aria-hidden="true" fill="currentColor" viewBox="0 0 20 20" xmlns="http://www.w3.org/2000/svg"><path fill-rule="evenodd" d="M13.477 14.89A6 6 0 015.11 6.524l8.367 8.368zm1.414-1.414L6.524 5.11a6 6 0 018.367 8.367zM18 10a8 8 0 11-16 0 8 8 0 0116 0z" clip-rule="evenodd" /></svg>`,
link: `<svg class="${iconClassName}" aria-hidden="true" fill="currentColor" viewBox="0 0 20 20" xmlns="http://www.w3.org/2000/svg"><path d="M11 3a1 1 0 100 2h2.586l-6.293 6.293a1 1 0 101.414 1.414L15 6.414V9a1 1 0 102 0V4a1 1 0 00-1-1h-5z" /><path d="M5 5a2 2 0 00-2 2v8a2 2 0 002 2h8a2 2 0 002-2v-3a1 1 0 10-2 0v3H5V7h3a1 1 0 000-2H5z" /></svg>`,
printer: `<svg class="${iconClassName}" aria-hidden="true" fill="currentColor" viewBox="0 0 20 20" xmlns="http://www.w3.org/2000/svg"><path fill-rule="evenodd" d="M5 4v3H4a2 2 0 00-2 2v3a2 2 0 002 2h1v2a2 2 0 002 2h6a2 2 0 002-2v-2h1a2 2 0 002-2V9a2 2 0 00-2-2h-1V4a2 2 0 00-2-2H7a2 2 0 00-2 2zm8 0H7v3h6V4zm0 8H7v4h6v-4z" clip-rule="evenodd" /></svg>`,

sharadsw and others added 3 commits August 6, 2024 10:04
Co-authored-by: Max Patiiuk <max@patii.uk>
Triggered by cdc8fbe on branch refs/heads/issue-2864
@sharadsw
Copy link
Contributor Author

sharadsw commented Aug 6, 2024

Removed the ban legacy icon

@maxpatiiuk
Copy link
Member

The current solution is good, but we need to test it:

  • does the text looks ok in other places (i.e workbench mapper and the data entry table list?)
  • if you change the font size and other UI settings, do icons still display fine, or not?

@sharadsw sharadsw marked this pull request as ready for review November 20, 2024 21:21
@sharadsw sharadsw requested a review from a team November 20, 2024 21:24
Copy link
Contributor

@pashiav pashiav left a comment

Choose a reason for hiding this comment

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

Testing instructions

  • Create a workbench dataset
  • Verify new icons are being used in the column headers
  • Verify icons work fine for new geo tables (eg: COG, COGType, age tables etc.)
  • Verify table icons look the same as before everywhere else (Data Entry, Schema Config etc.)
  • Verify icons look fine after changing font size or other text options

This looks so better now! Everything is working as expected.

Screenshot 2024-11-21 at 9 52 45 AM

@pashiav pashiav requested a review from a team November 21, 2024 15:55
Copy link
Contributor

@alesan99 alesan99 left a comment

Choose a reason for hiding this comment

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

Testing instructions

  • Create a workbench dataset
  • Verify new icons are being used in the column headers
  • Verify icons work fine for new geo tables (eg: COG, COGType, age tables etc.)
  • Verify table icons look the same as before everywhere else (Data Entry, Schema Config etc.)
  • Verify icons look fine after changing font size or other text options

chrome_sCQTYlN8ZL
The new icons are being used and look good 👍👍
Everywhere else where icons are used seemed to be functioning as usual

@sharadsw sharadsw merged commit 82ce523 into production Nov 21, 2024
12 checks passed
@sharadsw sharadsw deleted the issue-2864 branch November 21, 2024 17:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Old table icons are used in the WorkBench
6 participants