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

Datasets figure web component #886

Merged
merged 4 commits into from
Jun 26, 2024
Merged

Datasets figure web component #886

merged 4 commits into from
Jun 26, 2024

Conversation

fgravin
Copy link
Member

@fgravin fgravin commented May 28, 2024

Description

This PR introduces a new web component to display the amount of datasets within your catalog.
It's based on the <gn-figure> Angular component

Screenshots

image

Quality Assurance Checklist

  • Commit history is devoid of any merge commits and readable to facilitate reviews

This work is sponsored by Florent Gravin Enterprise.

Copy link
Contributor

github-actions bot commented May 28, 2024

Affected libs: feature-catalog, feature-record, feature-router,
Affected apps: datahub, webcomponents, metadata-editor, demo, map-viewer, search,

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

Copy link
Contributor

github-actions bot commented May 28, 2024

📷 Screenshots are here!

@fgravin fgravin requested a review from jahow June 6, 2024 08:13
constructor(injector: Injector, private changeDetector: ChangeDetectorRef) {
super(injector)
this.catalogRecords = injector.get(RecordsService)
this.recordsCount$ = this.catalogRecords.recordsCount$.pipe(startWith('-'))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would add a catchError(()=> of('-')) to stay consistent with the way you built it.
I like the way that you start with an '-'.
Since you put : catchError(() => of(0)) on the recordsCount$ in the service, in case of error it will display to the user 0 wich is wrong (could be 14 or 392).

What do you think about this ?

Copy link
Collaborator

@Angi-Kinas Angi-Kinas left a comment

Choose a reason for hiding this comment

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

LGTM 👍
I couldn't test it in the storybook locally because the build did not work.

@fgravin fgravin merged commit 49a5852 into main Jun 26, 2024
9 checks passed
@fgravin fgravin deleted the wc-figure-md-count branch June 26, 2024 18:10
@jahow jahow added this to the 2.4.0 milestone Jul 1, 2024
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.

4 participants