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

[Datahub]: Add a list layout to other links section when many links are present #873

Merged
merged 15 commits into from
May 16, 2024

Conversation

jahow
Copy link
Collaborator

@jahow jahow commented May 9, 2024

Description

This PR is a rework of #864.

Additional changes were:

  • rebased on main
  • made the carousel buttons bigger
  • added a new BlockListComponent, kind of like ListComponent of the above PR but taking any block as input
  • add a "compact" layout for the link card; this is used when links are shown as a list; the link card also has a title now
  • made all affected components standalone

@rcaplier let me know if these changes make sense considering your previous work. I realized that your changes regarding the layout of the carousel (letting it overflow by default) made the whole thing much simpler to use in the datahub. Also having a paginated list was not as trivial as I thought; I went with a generic BlockList component in order to not have a UI component that know too much about what is using it, but apart from that your original approach made sense.

Replaces #864

Screenshots

image

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 IGN.

Romuald Caplier and others added 5 commits May 9, 2024 00:04
step dots are now centered below but still absolute positioned
step dots have a larger interactive area
makes it standalone
adapt tests
Copy link
Contributor

github-actions bot commented May 9, 2024

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

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

Copy link
Contributor

github-actions bot commented May 9, 2024

📷 Screenshots are here!

jahow added 5 commits May 9, 2024 23:01
This component offers simple pagination on a list of blocks
Also do not include it in the ui-inputs module; as a standalone component,
it should be imported wherever this is necessary
@jahow jahow force-pushed the dh-other-links-refactoring-2 branch from 3346bdb to 06c88c8 Compare May 9, 2024 21:06
@coveralls
Copy link

coveralls commented May 9, 2024

Coverage Status

coverage: 81.323% (-0.6%) from 81.89%
when pulling 55e7abf on dh-other-links-refactoring-2
into 287bf00 on main.

@jahow jahow force-pushed the dh-other-links-refactoring-2 branch from 06c88c8 to 1ef846f Compare May 15, 2024 21:14
describe('set initial height as min height, keeps value when height changes', () => {
beforeEach(() => {
Object.defineProperties(component.blockContainer.nativeElement, {
clientHeight: {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we need to do this because jsdom doesn't actually compute the width and height of things; everything is always 0 in tests

@rcaplier
Copy link
Collaborator

I reviewed the code and beside the refactoring of the multiple async pipes into a single *ngrxLet obs$ as var to make the code more robust I didn't see anything wrong.

Good job ! I can't wait for it to be merged 😃

@jahow
Copy link
Collaborator Author

jahow commented May 16, 2024

@rcaplier thanks for the review! I added a commit to use ngrxLet and it does make things simpler 🙂

@jahow jahow merged commit c4b41b4 into main May 16, 2024
9 checks passed
@jahow jahow deleted the dh-other-links-refactoring-2 branch May 16, 2024 15:27
@jahow jahow mentioned this pull request May 16, 2024
7 tasks
@jahow
Copy link
Collaborator Author

jahow commented May 17, 2024

Closes #771

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