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 a proper tooltip for the Metadata Quality widget #952

Merged
merged 1 commit into from
Aug 1, 2024

Conversation

gkeimeHDF
Copy link
Contributor

@gkeimeHDF gkeimeHDF commented Jul 29, 2024

Description

This PR introduces a new Popover component

Architectural changes

The following library now depends on #819

  • Use tippy instead of create a custom tailwindCSS popover
  • Add popover component in ui
  • Use popover component in metadata-quality

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 MEL

@gkeimeHDF
Copy link
Contributor Author

@jahow let tell me if u have any comments

@jahow
Copy link
Collaborator

jahow commented Jul 30, 2024

Hi, thanks for this contribution. Could you please make sure that the CI is green first? thanks

@gkeimeHDF gkeimeHDF force-pushed the main branch 2 times, most recently from 6711ba8 to f554938 Compare July 30, 2024 10:14
@coveralls
Copy link

coveralls commented Jul 30, 2024

Coverage Status

coverage: 80.975% (-1.9%) from 82.903%
when pulling acc3f23 on gkeimeHDF:main
into 83f4887 on geonetwork:main.

@gkeimeHDF
Copy link
Contributor Author

gkeimeHDF commented Jul 30, 2024

@jahow i had seen one error then i had fixed it, now it should be fine. if u see another one just tell me

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.

Thanks @gkeimeHDF for creating a new Popover component with tippy.js!
This will make the UX much nicer :)

I added some comments. I think the export is the crucial thing to be added (and maybe the padding). Otherwise it looks good to me.

We could also re-use this new component for the favorite star component instead of tippy. But I'm not sure if this should be part of this PR.


@NgModule({
declarations: [
ColorScaleComponent,
ProgressBarComponent,
PopoverComponent,
Copy link
Collaborator

Choose a reason for hiding this comment

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

The PopoverComponent is missing in the exports Array otherwise it cannot be used by the metadata-quality component.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

export added

</div>
<ng-template #popoverItems>
<div class="mb-4 font-bold" translate>record.metadata.quality.details</div>
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 bit of padding to the content by adding a div around it with a class with some padding. For example:<div class="p-2 py-4">

This will make the component look a bit nicer:
from:
image
to:
image

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i will look to add a padding.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

padding added

export class PopoverComponent implements AfterViewInit, OnChanges, OnDestroy {
@ViewChild('popoverContent', { static: false }) popoverContent: ElementRef
@Input() content: string | TemplateRef<any>
@Input() theme: 'light' | 'light-border' | 'translucent' | 'material' | ''
Copy link
Collaborator

Choose a reason for hiding this comment

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

Somehow the theming doesn't seem to get applied. Not in the storybook nor in the datahub. Is there something I'm missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

my screenshoot come from the story-book using light-border. Your screenshoot looks like the default theming same as ''. I don't know yet why it doesn't apply on your side. Which browser do you use?
I had added only light-border on datahub and the differents on storybook.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Now the theming works on my side as well. Not sure what was the issue. Never mind! Looks good to me.

@Angi-Kinas
Copy link
Collaborator

The PR looks good to me, I'm just hesitant to approve because of the failing pipeline. The e2e are a bit flaky and sometimes pass after rerunning them.
I don't know about the Snyk Security part. @jahow do you know why it's failing?

@jahow
Copy link
Collaborator

jahow commented Jul 31, 2024

That's right, PRs coming from forks will have failures on some workflows (this is a known problem). As long as the E2E failures do not seem related to the PR it's fine to approve.

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.

Hey, thanks! I noticed the new UI component was not standalone, could you lease mak it so? thanks in advance

@Component({
selector: 'gn-ui-popover',
templateUrl: './popover.component.html',
styleUrls: ['./popover.component.css'],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you please make this component standalone? this is what we do for every new component now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what is standalone component, and what does it apply ? i had based my code on ProgressBarComponent which is not standalone

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jahow I have done it yersterday, and i fixed the lint error this morning should be ok now

@Angi-Kinas
Copy link
Collaborator

@gkeimeHDF Thanks for making the component standalone! In order to be able to use it in the metadata-quality component you need to import the PopoverComponent in the inputs array of libs/ui/elements/src/lib/ui-elements.module.ts. Thank you

@gkeimeHDF
Copy link
Contributor Author

gkeimeHDF commented Aug 1, 2024

@Angi-Kinas are you sure i need import in ui-elements? if it's standalone it should not be imported there no?. For sample PopupAlert is not imported in ui-widget.
Or i miss something?
@jahow

PS: And i had supposed u speak about libs/ui/widgets/src/lib/ui-widgets.module.ts. Maybe u really need speak about libs/ui/elements/src/lib/ui-elements.module.ts so are you sure i need import the component there if yes i can add but i don't see why i need to do this.

@Angi-Kinas
Copy link
Collaborator

Usually with standalone components you can easily import the component inside another component that uses it. E.g. the PopupAlertComponent is imported in the AutocompleteComponent. But since the metadata-quality component is not standalone we need to import it in libs/ui/elements/src/lib/ui-elements.module.ts where the MetadataQualityComponent is declared.
Does that make sense?

@gkeimeHDF
Copy link
Contributor Author

@Angi-Kinas Yes perfectly clear, i had changed the import.

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.

Perfect! The code looks good to me now :)
The e2e tests are still failing and one is related to the metadata-quality component.
I ran that particular e2e test locally and it passes.
@jahow can we still merge it?

@jahow
Copy link
Collaborator

jahow commented Aug 1, 2024

failures look unrelated :) @Angi-Kinas feel free to approve and merge, thanks!

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.

Thanks again to @gkeimeHDF for this feature!
I'll approve and merge.

@Angi-Kinas Angi-Kinas merged commit a1bcfe2 into geonetwork:main Aug 1, 2024
6 of 10 checks passed
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.

5 participants