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.

Sign up for GitHub

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

Jump to bottom

[ME]: Fix misc issues with dashboard view #990

Merged
merged 20 commits into from
Sep 26, 2024
Merged

[ME]: Fix misc issues with dashboard view #990

merged 20 commits into from
Sep 26, 2024

Conversation

rcaplier
Copy link
Collaborator

@rcaplier rcaplier commented Sep 16, 2024 •
edited by tkohr
Loading

Description

This PR fix many issues:

  • all collumns are now filled at loading
  • the width of the list of record always takes the whole page
  • several CSS fixes to match mockups (font size, weight, ...)
  • draft badge fixed (number properly centered and invisble when no draft)
  • pagination horizontaly centered

Screenshots

image

Quality Assurance Checklist

  • Commit history is devoid of any merge commits and readable to facilitate reviews
  • If new logic ⚙️ is introduced: unit tests were added
  • If new user stories 🤏 are introduced: E2E tests were added
  • If new UI components 🕹️ are introduced: corresponding stories in Storybook were created
  • If breaking changes 🪚 are introduced: add the breaking change label
  • If bugs 🐞 are fixed: add the backport <release branch> label
  • The documentation website 📚 has received the love it deserves

**This work is sponsored by **.

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Sep 16, 2024 •
edited
Loading

Affected libs: feature-editor, feature-router, feature-search, feature-map, feature-dataviz, feature-record, ui-inputs, ui-elements, feature-notifications, feature-catalog, ui-catalog, ui-search, ui-layout,
Affected apps: metadata-editor, datafeeder, demo, datahub, webcomponents, search, map-viewer, metadata-converter,

  • 🚀 Build and deploy storybook and demo on GitHub Pages
  • 📦 Build and push affected docker images

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Sep 16, 2024 •
edited
Loading

📷 Screenshots are here!

@rcaplier rcaplier force-pushed the me-fix-misc-issues-with-dashboard-views branch 5 times, most recently from 12b0e3c to 98c584d Compare September 19, 2024 13:13
@rcaplier rcaplier marked this pull request as ready for review September 19, 2024 13:13
@coveralls
Copy link

coveralls commented Sep 19, 2024 •
edited
Loading

Coverage Status

coverage: 81.399% (-0.8%) from 82.189%
when pulling 31aad0f on me-fix-misc-issues-with-dashboard-views
into e5f8e61 on main.

@rcaplier rcaplier force-pushed the me-fix-misc-issues-with-dashboard-views branch 5 times, most recently from ba93874 to a98e713 Compare September 20, 2024 14:06
@rcaplier rcaplier self-assigned this Sep 23, 2024
@jahow jahow force-pushed the me-fix-misc-issues-with-dashboard-views branch from a122918 to 8b33a3d Compare September 23, 2024 12:12
tkohr
tkohr reviewed Sep 24, 2024
View reviewed changes
tailwind.base.css Outdated Show resolved Hide resolved
tkohr
tkohr reviewed Sep 24, 2024
View reviewed changes
tailwind.base.css Outdated
Comment on lines 138 to 144
--rounded: var(--gn-ui-badge-rounded, 0.25em);
--padding: var(--gn-ui-badge-padding, 0.375em 0.75em);
--padding: var(--gn-ui-badge-padding, 0.375em 0.875em);
--text-color: var(--gn-ui-badge-text-color, var(--color-gray-50));
--background-color: var(--gn-ui-badge-background-color, black);
--opacity: var(--gn-ui-badge-opacity, 0.7);
@apply inline-block opacity-[--opacity] p-[--padding] rounded-[--rounded]
font-medium text-[length:0.875em] leading-none text-[color:--text-color] bg-[color:--background-color];
@apply opacity-[--opacity] p-[--padding] rounded-[--rounded]
font-medium text-[length:0.875em] text-[color:--text-color] text-xs bg-[color:--background-color] flex justify-center items-center content-center;
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should keep in mind that these changes affect all badges in gn-ui including the datahub.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I checked the datahub and badges seems ok.

@tkohr tkohr mentioned this pull request Sep 24, 2024
Merged
8 tasks
@jahow
Copy link
Collaborator

jahow commented Sep 25, 2024 •
edited
Loading

@tkohr I did some more tests and found these suggestions:

  • could we have at least an empty bar on top of the my-drafts screen? otherwise the layout jumps when navigating to this page
  • could we have the notifications container as part of the Dashboard page component and not of individual pages?
  • also the scroll issue on the dashboard page is still there :)
  • the table on the my-drafts page still has a bigger font size
  • when searching on the my-records page the title is not changed, whereas it is changed on the all-records page:

image

image

@tkohr
Copy link
Collaborator

tkohr commented Sep 25, 2024

Thanks for the review @jahow! I've addressed the points above, hoping it looks good to merge now :-)

@jahow
Copy link
Collaborator

jahow commented Sep 25, 2024

@tkohr wait, before merging could you please change back the text size of the gn-ui-badge to what it was before? and then use a smaller text size in the editor when needed? otherwise the keywords look too small in the datahub now

@tkohr
fix(badge): adapt dashboard badge font sizes and padding
ac0f1dd 
and revert generic badge font size and padding
@tkohr
Copy link
Collaborator

tkohr commented Sep 25, 2024 •
edited
Loading

@tkohr wait, before merging could you please change back the text size of the gn-ui-badge to what it was before? and then use a smaller text size in the editor when needed? otherwise the keywords look too small in the datahub now

Done. Note that I had to make the font-size configurable as well. Couldn't get it done with [style.font-size]="'12px'". I also noticed that the format tag (that does not use the badge) did not use the right font size and adapted this along the way, oriented on other mockup badges in the table as I did not see any for formats. I let you approve/merge if it looks ok.

jahow
jahow approved these changes Sep 25, 2024
View reviewed changes
Copy link
Collaborator

@jahow jahow left a comment

Choose a reason for hiding this comment

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

Great work, thanks to both of you @tkohr and @rcaplier! I'm approving, feel free to merge when you think it's ready

@tkohr tkohr merged commit f501972 into main Sep 26, 2024
13 checks passed
@tkohr tkohr deleted the me-fix-misc-issues-with-dashboard-views branch September 26, 2024 08:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tkohr tkohr tkohr left review comments

@jahow jahow jahow approved these changes

Assignees

@rcaplier rcaplier

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rcaplier @coveralls @jahow @tkohr