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

Metadata Editor: rework search results layout, add "create record" button #805

Merged
merged 13 commits into from
Mar 1, 2024

Conversation

jahow
Copy link
Collaborator

@jahow jahow commented Feb 25, 2024

Description

This PR affects the layout of the search results in the dashboard views of the editor:

  • Removed /records/all, only keep /records/search route (similar to the datahub)
  • Adapt the layout of the search results in the dashboard to make it closer to the mockups
  • Create a new RecordsCountComponent class to extract the part that shows the amount of records and selected records in the table
  • Add a "create record" button on the search results view
  • Removed unused CreatePageComponent, now EditComponent is used both for editing and creating
  • Document some best practices related to standalone components and testing

Architectural changes

Add ngx-translate-testing dependency (instead of a local copy of the lib that was introduced while waiting for rxJS 7)

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

Copy link
Contributor

github-actions bot commented Feb 25, 2024

Affected libs: common-domain, api-metadata-converter, api-repository, feature-catalog, feature-record, feature-router, feature-search, feature-map, feature-dataviz, feature-auth, feature-editor, ui-search, common-fixtures, ui-elements, ui-catalog, util-shared, ui-widgets, ui-inputs, ui-layout, ui-map, ui-dataviz, util-i18n, util-app-config, data-access-datafeeder, util-data-fetcher, data-access-gn4,
Affected apps: datahub, metadata-editor, webcomponents, metadata-converter, datafeeder, demo, map-viewer, search, data-platform,

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

@jahow jahow marked this pull request as ready for review February 25, 2024 20:45
@coveralls
Copy link

coveralls commented Feb 25, 2024

Coverage Status

coverage: 84.063% (+0.6%) from 83.444%
when pulling bf20692 on ME/rework-layout
into 5b8d393 on main.

Copy link
Collaborator

@LHBruneton-C2C LHBruneton-C2C left a comment

Choose a reason for hiding this comment

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

Thanks for this great work!

Comment on lines +16 to +19
<h1 class="text-[16px] text-main font-title font-bold" translate>
dashboard.records.all
</h1>
<div class="text-[12px]">
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't mind for gap, padding, margin, but should text size be hardcoded in pixels this way?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Honestly I always hardcode pixel values like that now, then there's no need to look for the meaning of e.g. text-sm in pixels (also many TW classes use rem instead of px which is one more annoying thing when working with precise mockups).

I don't see any drawbacks to this personally, do you think this could be an issue?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It depends, do you intend to be responsive when people change there screen size, zoom in/out?
I honestly don't know how Tailwind CSS behaves in those cases.

Copy link
Collaborator Author

@jahow jahow Feb 29, 2024

Choose a reason for hiding this comment

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

TW has modifiers for screen sizes so there's really no impact here 🙂
edit: https://tailwindcss.com/docs/responsive-design

docs/guide/best-practices.md Show resolved Hide resolved
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was removed because it wasn't working and it basically needs a whole application to run, which doesn't really make sense in the context of storybook anymore

@jahow jahow merged commit 2795595 into main Mar 1, 2024
9 checks passed
@jahow
Copy link
Collaborator Author

jahow commented Mar 1, 2024

Thanks for the review!

@jahow jahow deleted the ME/rework-layout branch March 1, 2024 08:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants