[Sponsor by MEL] Add feature Metadata Quality Widget #497

gkeimeHDF · 2023-05-30T08:21:55Z

fgravin

Thanks @gkeimeHDF for the contribution !

It looks overall quite good.

I've just done a very quick first review, more about the "forme" than the "fond" 😄
I will deeply test it and review it next week, from there, you can still start with

rebase your work on the last main
add tests to your UI components
add tests to the components that you've changed (to check if the widget is present or not for instance)
add stories to your UI components
address the first comments

Note that it's easier if you name your PR branch with a description of what it's about, rather than using the main tag.

In term of rendering, I really think that the progress bar component is not adapted for the metadata quality widget, it should stick to something similar to what we have on data.gouv.

Here is a first mockup about what the customer wants for the design, it's way better integrated and discret.

We will test the behavior and the computation better next week, thanks again for your great work.
Cheers

apps/search/src/assets/i18n/en.json

libs/ui/elements/src/lib/metadata-quality-info/metadata-quality-info.component.ts

fgravin · 2023-06-15T07:47:27Z

libs/ui/elements/src/lib/metadata-quality-info/metadata-quality-info.component.ts

+  @Input() value: boolean
+
+  get display() {
+    const name_snake_upper = this.name.replace(/[A-Z]/g, (letter) => `_${letter.toLowerCase()}`).toUpperCase();


Variable name must be camel case.

libs/ui/elements/src/lib/metadata-quality/metadata-quality.component.css

libs/ui/elements/src/lib/metadata-quality/metadata-quality.component.spec.ts

libs/ui/elements/src/lib/metadata-quality/metadata-quality.component.ts

fgravin · 2023-07-04T14:17:58Z

Hello @gkeimeHDF, any news on the topic, regarding my first bunch of comments ?
Cheers

gkeimeHDF · 2023-07-05T08:12:10Z

@fgravin i was waiting some time if u have more comments, looks like it was completed. Today i had fixed all the issues.

Have a good day

fgravin · 2023-07-05T08:15:52Z

@fgravin i was waiting some time if u have more comments, looks like it was completed. Today i had fixed all the issues.

Ok no worry sorry, we haven't tested it yet deeply.
Thanks for the update, we'll have a look shortly.
Did you change the ui component, not to use the default progress bar which is not satisfying in terms of rendering ?
Best.

gkeimeHDF · 2023-07-05T08:21:27Z

No because it's the progress bar is the one that MEL has choosen, the meter component was denied by them.

Best regards

Guillaume

fgravin · 2023-07-05T08:26:18Z

No because it's the progress bar is the one that MEL has choosen, the component was denied by them.

The screenshot that I provided above comes from MEL UX/UI work, it is what they expect.
I would suggest that you talk with Florent Berault about that, to see if their need has changed or not since he talked about the actual progress bar.
Thanks !

fgravin

Thanks for the enhancements.
I've done a deeper review and unfortunately there is an architecture issue with the way your dumb components initiate their state (from the config).

I've made several comments as well.
Also, please add the documentation in the datahub readme for now, otherwise, people won't know how to use the new fields to sort on the quality.

Thanks a lot.

fgravin · 2023-07-12T10:19:17Z

apps/datahub/src/styles.css

@@ -7,6 +7,9 @@ body {
  margin: 0;
 }

+.container-xs {


I wonder if this shouldn't be supported by https://tailwindcss.com/docs/container

.container-sm shouldn't exist (it's our responsibility to remove it), do not take it as an exemple.

On this comment, we should do something or let the code like this?

You should remove the code because we try as much as possible to avoid having global CSS (which is very hard to maintain in the long run). For styling you can either use tailwind classes or, if styling is too elaborate, use the component stylesheet, which is encapsulated.

i can remove the code but when we will reduce the screen the the title can overflow above the widget. Is it ok? @jahow

No, we still need to have a working layout

fgravin · 2023-07-12T10:21:33Z

conf/default.toml

+# if you add an indexed field to calculate the qualityScore, the datahub search allow you to sort on this field with this parameter
+# sortable = true
+
+# by default the wudget appear in 2 location in search list and in detail page


Suggested change

# by default the wudget appear in 2 location in search list and in detail page

# by default the widget appears in 2 locations: in the search list and in the detail page

fgravin · 2023-07-12T10:22:11Z

conf/default.toml

+# by default the wudget appear in 2 location in search list and in detail page
+# display_widget_in_detail = false // allow you to hide the widget in detail
+# display_widget_in_search = false // allow you to hide the widget in search list
+# If you want see the widget in the two location, don't fill theses configurations


Suggested change

# If you want see the widget in the two location, don't fill theses configurations

# If you want see the widget in the two locations, don't fill theses configurations

fgravin · 2023-07-12T10:34:35Z

libs/feature/record/src/lib/record-metadata/record-metadata.component.ts

@@ -60,7 +62,11 @@ export class RecordMetadataComponent {
    private searchService: SearchService,
    private sourceService: SourcesService,
    private orgsService: OrganisationsServiceInterface
-  ) {}
+  ) { }


the formatting is wrong, you should install a prettier plugin within your IDE to auto format the files.
just run a npm run format to fix all formatting at once.

fgravin · 2023-07-12T12:18:27Z

libs/feature/search/src/lib/utils/mapper/elasticsearch.field.mapper.ts

+    MD_LegalConstraintsUseLimitationObject: (output, source) => {
+      const legalConstraints = getAsArray(
+        selectField<SourceWithUnknownProps[]>(source, 'MD_LegalConstraintsUseLimitationObject')
+      ).map((MD_LegalConstraintsUseLimitationObject) => selectTranslatedValue<string>(MD_LegalConstraintsUseLimitationObject));


variables should always be camel case.

fgravin · 2023-07-12T13:47:14Z

libs/ui/elements/src/lib/metadata-quality-info/metadata-quality-info.component.ts

+})
+export class MetadataQualityInfoComponent {
+
+  metadataQualityConfig: MetadataQualityConfig = getMetadataQualityConfig();


ui library, even every library are not allow to import the config.
an application could run without the configuration, it means that no dependency are allowed, configuration is only available from the applications.
We have set up linting rules for that, I would suggest you to run npm run lint before pushing a PR to check that everything is correct. The CI should do it I will enable it on your PR.

This means that you'll have to refactor your approach. You should read the configuration, extract meaningful informations out of it, and pass those flags to the dedicated components like "displayQualityInRecordPage" and drill them in.

The config is not required because there is a default config

fgravin · 2023-07-12T13:48:59Z

libs/ui/elements/src/lib/metadata-quality-info/metadata-quality-info.component.ts

+  @Input() name: string
+  @Input() value: boolean
+
+  get display() {


I think that the display should be handled outside of the component as well

Where do you wish this display should be handled? @jahow @fgravin

fgravin · 2023-07-12T13:49:21Z

libs/ui/elements/src/lib/metadata-quality/metadata-quality.component.css

+  font-size: 15px;
+}
+
+.menu {


should be done with tailwind CSS, no css is needed here

Try to remove all css properties here and use tailwind classes with the HTML template instead.

fgravin · 2023-07-12T13:52:32Z

libs/ui/elements/src/lib/metadata-quality/metadata-quality.component.spec.ts

+  it('content', () => {
+    expect(component.metadata?.contact?.organisation).toBe("Ifremer");
+  })
+})


You should test the sub component inputs as well with component mocks.

@Component({ selector: 'gn-ui-metadata-quality-info', template: '', }) class MockMetadataQualityInfoComponent { @Input() name: string; @Input() value: any; } .. await TestBed.configureTestingModule({ declarations: [MetadataQualityComponent, MockMetadataQualityInfoComponent], }).compileComponents(); ... component.metadata = { qualityScore: 80, // Sample metadata for testing title: 'Sample Title', abstract: 'Sample Abstract', topic: ['Topic 1', 'Topic 2'], keywords: ['Keyword 1', 'Keyword 2'], legalConstraints: ['Legal Constraint 1', 'Legal Constraint 2'], contact: { organisation: 'Sample Organization', email: '[email protected]', }, updateFrequency: 'Monthly', }; ... it('should display sub-components with correct inputs', () => { const metadataInfoElements = fixture.nativeElement.querySelectorAll('gn-ui-metadata-quality-info'); expect(metadataInfoElements.length).toBe(8); const titleElement = metadataInfoElements[0]; expect(titleElement.componentInstance.name).toBe('title'); expect(titleElement.componentInstance.value).toBe('Sample Title'); const abstractElement = metadataInfoElements[1]; expect(abstractElement.componentInstance.name).toBe('description'); expect(abstractElement.componentInstance.value).toBe('Sample Abstract'); const topicElement = metadataInfoElements[2]; expect(topicElement.componentInstance.name).toBe('topic'); expect(topicElement.componentInstance.value).toBe(true); const keywordsElement = metadataInfoElements[3]; expect(keywordsElement.componentInstance.name).toBe('keywords'); expect(keywordsElement.componentInstance.value).toBe(true); const legalConstraintsElement = metadataInfoElements[4]; expect(legalConstraintsElement.componentInstance.name).toBe('legalConstraints'); expect(legalConstraintsElement.componentInstance.value).toBe(true); const organizationElement = metadataInfoElements[5]; expect(organizationElement.componentInstance.name).toBe('organisation'); expect(organizationElement.componentInstance.value).toBe('Sample Organization'); const contactElement = metadataInfoElements[6]; expect(contactElement.componentInstance.name).toBe('contact'); expect(contactElement.componentInstance.value).toBe('[email protected]'); const updateFrequencyElement = metadataInfoElements[7]; expect(updateFrequencyElement.componentInstance.name).toBe('updateFrequency'); expect(updateFrequencyElement.componentInstance.value).toBe('Monthly'); });

i just need to put this snippet in the test page ?

the snippet is just intended as an example, I don't think it'll work as-is. The idea is that you test things in a more complete way by checking that the children component get the right inputs for each field in the list.

fgravin · 2023-07-12T13:54:40Z

libs/util/app-config/src/lib/app-config.ts

+              DISPLAY_ORGANISATION: parsedMetadataQualitySection.display_organisation,
+        } as MetadataQualityConfig)
+
+      searchConfig =


why do you have code about the geometry ?

there is no geometry? Nothing to do here?

the lines 249 from 255 look like they were duplicated by mistake, as they are about the filter geometry parameter

fgravin · 2023-07-24T12:01:32Z

conf/default.toml

@@ -54,6 +54,9 @@ background_color = "#fdfbff"
 # title_font = "'My Custom Title Font', fallback-font-title"
 # fonts_stylesheet_url = "https://fonts.googleapis.com/css2?family=Open+Sans:wght@400;700&family=Permanent+Marker&display=swap"

+# This optional parameter allow you to change the default class on progress bar, by default is font-bold
+# progress_bar_text_class = 'font-normal'


I forgot this point but we don't want to introduce this kind of config. You can just get rid of it.
Thanks

I need remove the code? did we change the default font by font-normal and let it to font-bold?

I think you can remove this change and enforce the font weight of the progress bar from the parent component.

One solution is to use CSS variables in the progress bar component, such as:

.label { font-weight: var(--progress-bar-font-weight, 'bold'); }

And then from the parent component:

:host { --progress-bar-font-weight: 'normal'; }

See a concrete example with this component: https://github.com/geonetwork/geonetwork-ui/blob/main/libs/ui/inputs/src/lib/star-toggle/star-toggle.component.css

i can revert my code and let it like before without change the font-weight and let it bold for all in tailwind css. Is it ok ? @jahow

No I think you should find a solution like the one I suggested

gkeimeHDF · 2023-08-01T09:35:17Z

Snyk security is failed but it's not from the code provided ?

jahow · 2023-08-01T09:59:22Z

Snyk security is failed but it's not from the code provided ?

No that's probably because the snyk workflow is lacking permission.

jahow · 2023-08-01T12:09:47Z

libs/feature/search/src/lib/sort-by/sort-by.component.ts

@@ -3,10 +3,12 @@ import { marker } from '@biesbjerg/ngx-translate-extract-marker'
 import { SortByEnum } from '@geonetwork-ui/util/shared'
 import { SearchFacade } from '../state/search.facade'
 import { SearchService } from '../utils/service/search.service'
+import { getMetadataQualityConfig } from '@geonetwork-ui/util/app-config'


We try to avoid importing the app config from libs because components can exist without an app configuration (e.g. in web components); in this situation such an import might cause a crash.

See other comment by @fgravin

jahow · 2023-08-01T12:12:44Z

libs/feature/search/src/lib/utils/mapper/elasticsearch.field.mapper.ts

+    if (check('TITLE')) {
+      if (selectField(source, 'resourceTitleObject')) {
+        success++;
+      }
+    }


nested ifs can be combined 🙂

jahow · 2023-08-01T12:20:13Z

Big thanks for your continued work on this @gkeimeHDF, I have added a few comments to hopefully make things clearer for you. I feel like we're getting close to having something mergeable! Did you manage to run the lint and format tasks?

Edit: also keep in mind that the PR will need to be rebased before merging, as conflicts have appeared with the main branch, thanks!

gkeimeHDF · 2023-08-01T12:40:11Z

@jahow How to launch lint and format ?

I tried :
npm run format

> [email protected] format
> nx format:write
tsconfig.base.json

npm run lint

> [email protected] lint
> nx affected:lint


 >  NX   Affected criteria defaulted to --base=remotes/origin/main --head=HEAD

 >  NX   Successfully ran target lint for 0 projects (3ms)
 
   View logs and run details at https://nx.app/runs/rqIj8pipxz

f-necas · 2023-08-01T14:02:55Z

Hi @gkeimeHDF,

npm run lint and npm run format should work. Do you execute them on geonetwork-ui/ root folder ?

gkeimeHDF · 2023-08-01T18:29:05Z

Hi @gkeimeHDF,

npm run lint and npm run format should work. Do you execute them on geonetwork-ui/ root folder ?

yes else i will got error if no package.json is present.

f-necas · 2023-08-02T10:19:54Z

Indeed, my bad.

Maybe you could have a try with npm run lint:all and for format directly with npx nx format:write ? That's just some suppositions.

We have released the 1.1.0 version this morning, you can rebase on it.

Let me know if it works !

jahow · 2023-08-25T18:49:26Z

Hi, #464 was just merged and it changes significantly the internal data format used in geonetwork-ui. Please use this issue if you need any pointers or indications on how to adapt your work consequently, thanks.

gkeimeHDF · 2023-09-04T13:17:16Z

@jahow @fgravin

Hello, i submit a new version about the quality widget code that contains yours comments.

Can you have a look and tell me if u have some others comments?

Thank you

fgravin · 2023-09-05T08:09:41Z

Can you have a look and tell me if u have some others comments?

Hi @gkeimeHDF sure we'll have a look today or tomorrow, thanks for your work.

fgravin

Thanks for the refactoring @gkeimeHDF it looks very good.
I have made some minor comments for the final adjustments.

There is so much prop drilling that I wonder if a service wouldn't have been more appropriate... This is good enough though.

Thanks 👍

fgravin · 2023-09-07T15:06:08Z

libs/api/repository/src/lib/gn4/elasticsearch/constant.ts

@@ -11,7 +11,14 @@ export const ES_SOURCE_SUMMARY = [
  'linkProtocol',
  'contactForResource.organisation',
  'contact.organisation',
+  'contact.email',


This change is redundant with FIELDS_SUMMARY.
I think that ES_SOURCE_SUMMARY should be removed, what do you think @jahow .

@gkeimeHDF it's not your responsability, but I don't think that you need this change, as this constant is just used in the related records request.

no big deal, there's probably some more cleanup to do but that doesn't have to be part of this PR; I'm fine either way

fgravin · 2023-09-07T15:08:50Z

libs/common/domain/src/lib/record/metadata.model.ts

@@ -58,10 +58,14 @@ export interface BaseRecord {
  keywords: Array<string> // TODO: handle thesaurus and id
  accessConstraints: Array<AccessConstraint>
  useLimitations: Array<string>
+  legalConstraints?: Array<string>


I don't know what to think with these changes, you are updating the pivot format which has been carefuly designed from various standards. I let @jahow judge, maybe those properties were missing.

BTW How do we manage topic over themes ?

legal constraints are accessConstraints of type legal, so no need for a new field I would say

Topics and themes are semantically the same thing I would say. In ISO they are in two different places but we can probably append them together in the themes array for now. Non-INSPIRE records will get their themes from the cl_topic field, whereas INSPIRE records will get them from the inspireTheme field.

@jahow legalConstraints doesn't go to accessConstraints, it go to useLimitations which is a string Array.

https://github.com/geonetwork/geonetwork-ui/blob/7a84a5b053a3d56ae57f563d8793a40b4cca528f/libs/api/metadata-converter/src/lib/gn4/gn4.field.mapper.ts#L248C12-L248C12

The field is MD_LegalConstraintsUseLimitationObject

@jahow Des nouvelles sur le sujet?

@jahow @fgravin i still need an answer about legalConstraints. Should i use accessConstraints type "legal" then actually it won't works because there no accessContraints for this field but a useLimitations ?

@gkeimeHDF sorry for taking so long to answer. It's a tricky subject, I think we can leave it like that and improve it later on. Thanks!

fgravin · 2023-09-07T15:12:53Z

libs/feature/search/src/lib/sort-by/sort-by.component.ts

@@ -34,6 +39,12 @@ export class SortByComponent {
    private searchService: SearchService
  ) {}

+  ngOnInit(): void {
+    if (!this.isQualitySortable) {


I would prefer that you do the opposite, cause this method won't work if we want a setting for another sort property.

If it's enable, then add the option in the choices array.

fgravin · 2023-09-07T15:17:22Z

libs/ui/elements/src/lib/metadata-quality/metadata-quality.component.css

+  font-size: 15px;
+}
+
+.menu {


Try to remove all css properties here and use tailwind classes with the HTML template instead.

fgravin · 2023-09-07T15:25:39Z

libs/ui/elements/src/lib/metadata-quality/metadata-quality.component.ts

+  }
+
+  ngOnChanges(changes: SimpleChanges): void {
+    if (changes['metadata'] || changes['metadataQualityDisplay'])


Does it enter here (except on component init) ? I don't understand how this component could see its input changed 🤔

Ok it does on the metadata view, because we first load a ghost page with a small metadata object, and when the full object is loaded, we hydrate the content with the new object.

Anyway, ngOnChanges is also called on init so there is a redundancy here, remove the ngOnInit method.

I would load the widget only when the full metadata object is fetched, not on pre-loading though.

fgravin · 2023-09-07T15:42:46Z

libs/ui/elements/src/lib/ui-elements.module.ts

@@ -50,6 +52,8 @@ import { PaginationButtonsComponent } from './pagination-buttons/pagination-butt
    RelatedRecordCardComponent,
    MetadataContactComponent,
    MetadataCatalogComponent,
+    MetadataQualityComponent,
+    MetadataQualityInfoComponent,


Honestly, it's hard to understand at first sight that MetadataQualityInfoComponent is one item of the MetadataQualityComponent component, the name is not talkative enough IMO.

fgravin · 2023-09-07T15:48:32Z

libs/ui/widgets/src/lib/progress-bar/progress-bar.component.css

@@ -0,0 +1,3 @@
+.text-4 {


why not keeping font-bold ?

jahow

Added some comments, I hope it clarifies things a bit. We're almost there @gkeimeHDF, thanks for the continued work.

jahow · 2023-09-07T21:14:17Z

libs/common/domain/src/lib/record/metadata.model.ts

  licenses: Array<License>
  overviews: Array<GraphicOverview>
  extras?: Record<string, unknown>
  landingPage?: URL
+  topic?: Array<string>
+  updateFrequency?: UpdateFrequency
+  qualityScore?: number


This should go in extras I think

I don't understand this comment, can you more details?

So the mapper can output things that are too specific for the pivot format in an extras field, see for example:

geonetwork-ui/libs/api/metadata-converter/src/lib/gn4/gn4.field.mapper.ts

Lines 205 to 211 in 7a84a5b

userSavedCount: (output, source) =>

this.addExtra(

{

favoriteCount: parseInt(selectField(source, 'userSavedCount')),

},

output

),

I think you should also use addExtra for the qualityScore field

jahow · 2023-09-07T21:15:11Z

libs/common/domain/src/lib/record/metadata.model.ts

@@ -58,10 +58,14 @@ export interface BaseRecord {
  keywords: Array<string> // TODO: handle thesaurus and id
  accessConstraints: Array<AccessConstraint>
  useLimitations: Array<string>
+  legalConstraints?: Array<string>


legal constraints are accessConstraints of type legal, so no need for a new field I would say

jahow · 2023-09-07T21:18:17Z

libs/api/repository/src/lib/gn4/elasticsearch/constant.ts

@@ -11,7 +11,14 @@ export const ES_SOURCE_SUMMARY = [
  'linkProtocol',
  'contactForResource.organisation',
  'contact.organisation',
+  'contact.email',


no big deal, there's probably some more cleanup to do but that doesn't have to be part of this PR; I'm fine either way

jahow · 2023-09-07T21:23:39Z

libs/common/domain/src/lib/record/metadata.model.ts

@@ -58,10 +58,14 @@ export interface BaseRecord {
  keywords: Array<string> // TODO: handle thesaurus and id
  accessConstraints: Array<AccessConstraint>
  useLimitations: Array<string>
+  legalConstraints?: Array<string>


Topics and themes are semantically the same thing I would say. In ISO they are in two different places but we can probably append them together in the themes array for now. Non-INSPIRE records will get their themes from the cl_topic field, whereas INSPIRE records will get them from the inspireTheme field.

jahow · 2023-09-07T21:24:08Z

libs/common/domain/src/lib/record/metadata.model.ts

  licenses: Array<License>
  overviews: Array<GraphicOverview>
  extras?: Record<string, unknown>
  landingPage?: URL
+  topic?: Array<string>


See comment above, this new field should not be necessary.

gkeimeHDF · 2023-09-25T17:24:23Z

@jahow @fgravin i did a new version taking account the last comments

fgravin · 2023-10-18T08:18:18Z

@jahow @fgravin i did a new version taking account the last comments

Thank you @gkeimeHDF, we have been busy with the 2.0.0 release sorry for the delay.
We'll do a last review and should merge it straight after 🙏

fgravin · 2023-10-18T09:45:40Z

@gkeimeHDF could you please fix the issues pointed by the CI ?
I would suggest that you run the format,lint,test commands locally before pushing.
Thanks

gkeimeHDF · 2023-10-18T10:01:23Z

@gkeimeHDF could you please fix the issues pointed by the CI ? I would suggest that you run the format,lint,test commands locally before pushing. Thanks

@fgravin i had already runned format lint and test without issue. Which is the issue?

fgravin · 2023-10-18T10:45:15Z

The CI is failing there https://github.com/geonetwork/geonetwork-ui/actions/runs/6302671953/job/17815157041#step:6:13

gkeimeHDF · 2023-10-18T10:50:16Z

The CI is failing there https://github.com/geonetwork/geonetwork-ui/actions/runs/6302671953/job/17815157041#step:6:13

Yes i see, it works in local without prompt any errors, so i guess you need to fix the issue in your ci.

fgravin · 2023-10-18T12:02:28Z

The CI is failing there https://github.com/geonetwork/geonetwork-ui/actions/runs/6302671953/job/17815157041#step:6:13

Yes i see, it works in local without prompt any errors, so i guess you need to fix the issue in your ci.

The CI rarely lies 😉

If you run npm run format:check you will see the list of invalid files, if there is at least one file in the list, the CI fails.
Run npm run format to autofix the formatting. Usually, it's recommended to configure your code editor with prettier so the formatting is automatic.

Thanks

gkeimeHDF · 2023-10-18T12:28:55Z

The CI is failing there https://github.com/geonetwork/geonetwork-ui/actions/runs/6302671953/job/17815157041#step:6:13

Yes i see, it works in local without prompt any errors, so i guess you need to fix the issue in your ci.

The CI rarely lies 😉

If you run npm run format:check you will see the list of invalid files, if there is at least one file in the list, the CI fails. Run npm run format to autofix the formatting. Usually, it's recommended to configure your code editor with prettier so the formatting is automatic.

Thanks

Did you try the branch ? i had even downgraded my node version to match your ci node version without any errors.

npm run format:check --verbose
npm info using [email protected]
npm info using [email protected]
npm verb title npm run format:check
npm verb argv "run" "format:check" "--loglevel" "verbose"
npm verb logfile logs-max:10 dir:/Users/kembris/.npm/_logs/2023-10-18T11_05_07_512Z-
npm verb logfile /Users/kembris/.npm/_logs/2023-10-18T11_05_07_512Z-debug-0.log

> [email protected] format:check
> nx format:check

npm verb exit 0
npm info ok 
geonetwork-ui % npm run format                

> [email protected] format
> nx format:write

tsconfig.base.json
geonetwork-ui % git status
Sur la branche main
Votre branche est à jour avec 'origin/main'.

rien à valider, la copie de travail est propre

i would suggest you to add --verbose to view what are the issue from your CI.

fgravin · 2023-10-18T12:40:08Z

I fetched your fork, and set me on detached HEAD on your branch. Then running the format gives me errors (the exact same files as in the CI):

fgravin@wkr19:~/dev/geonetwork-ui$ git st
HEAD detached at neogeo/main
no changes added to commit (use "git add" and/or "git commit -a")

fgravin@wkr19:~/dev/geonetwork-ui$ npx nx format:check
libs/api/repository/src/lib/gn4/elasticsearch/constant.ts
libs/api/repository/src/lib/gn4/elasticsearch/elasticsearch.service.spec.ts
libs/ui/search/src/lib/results-list-item/results-list-item.component.ts
fgravin@wkr19:~/dev/geonetwork-ui$ git branch

Are you sure that you pushed everything ? Don't you have anything in your stage ? Would be weird if not.

gkeimeHDF · 2023-10-18T12:57:33Z

Yes all is pushed i don't have issue.

I did:
git clone https://github.com/gkeimeHDF/geonetwork-ui.git
cd geonetwork-ui

npm install then:

% npx nx format:check

% git status
Sur la branche main
Votre branche est à jour avec 'origin/main'.
rien à valider, la copie de travail est propre

% git push
Everything up-to-date

fgravin · 2023-10-18T13:19:37Z

Ok, weird. I can't see any reason why the format command passes on your environment while the formatting of some files are obviously wrong (cf gkeimeHDF#1).

Anyway, to move forward, let's merge my PR and re-run the CI.
Thanks

Fix formatting

gkeimeHDF · 2023-10-18T14:03:58Z

i have merged the PR, maybe the formatter had an issue on macos ?

f-necas · 2023-10-18T18:36:59Z

i have merged the PR, maybe the formatter had an issue on macos ?

Hello @gkeimeHDF, what IDE do you use ? In IntelliJ, I had to update the Prettier configuration.

gkeimeHDF · 2023-10-19T07:31:28Z

@f-necas i use vscode with macos

fgravin · 2023-10-19T07:35:05Z

This is not an issue about the IDE, cause it does not work apparently from the command line either.
I really think that it did work on your environment cause otherwise it would be impossible to have so few formatting errors from your PR.
Maybe the version of prettier, or nx, or prettier is confused about the configuration it uses ... ¯\_(ツ)_/¯

gkeimeHDF · 2023-10-19T07:41:43Z

npx prettier --version
2.7.1

jahow

All green except Snyk which is a problem that is on the CI, not on this PR

Thanks @gkeimeHDF for the ongoing work

fgravin reviewed Jun 15, 2023

View reviewed changes

gkeimeHDF force-pushed the main branch from 5005de3 to 00e9c4b Compare July 5, 2023 08:02

fgravin added enhancement Proposal for a new feature major feature labels Jul 5, 2023

fgravin requested changes Jul 12, 2023

View reviewed changes

fgravin reviewed Jul 24, 2023

View reviewed changes

jahow reviewed Aug 1, 2023

View reviewed changes

gkeimeHDF force-pushed the main branch from 00e9c4b to d7ebdcd Compare August 21, 2023 09:10

gkeimeHDF force-pushed the main branch 4 times, most recently from 47bae6b to 258ebf7 Compare September 4, 2023 13:04

fgravin self-assigned this Sep 7, 2023

fgravin linked an issue Sep 7, 2023 that may be closed by this pull request

Add a metadata quality widget #610

Closed

fgravin reviewed Sep 7, 2023

View reviewed changes

jahow reviewed Sep 7, 2023

View reviewed changes

gkeimeHDF force-pushed the main branch 2 times, most recently from 83eaea6 to f6ac8b4 Compare September 11, 2023 09:26

gkeimeHDF force-pushed the main branch from f6ac8b4 to 992f62f Compare September 25, 2023 17:15

rebase main + add quality

4b4552c

gkeimeHDF force-pushed the main branch from 992f62f to 4b4552c Compare September 25, 2023 17:17

fix(format)

5a52a41

Merge pull request #1 from fgravin/fix-format

dddbe48

Fix formatting

fgravin added this to the 2.1.0 milestone Oct 18, 2023

jahow approved these changes Oct 25, 2023

View reviewed changes

jahow merged commit 2901ae6 into geonetwork:main Oct 25, 2023
5 of 6 checks passed

	# by default the wudget appear in 2 location in search list and in detail page
	# by default the widget appears in 2 locations: in the search list and in the detail page

	# If you want see the widget in the two location, don't fill theses configurations
	# If you want see the widget in the two locations, don't fill theses configurations

	userSavedCount: (output, source) =>
	this.addExtra(
	{
	favoriteCount: parseInt(selectField(source, 'userSavedCount')),
	},
	output
	),

[Sponsor by MEL] Add feature Metadata Quality Widget #497

[Sponsor by MEL] Add feature Metadata Quality Widget #497

Conversation

gkeimeHDF commented May 30, 2023 • edited Loading

fgravin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fgravin commented Jul 4, 2023

gkeimeHDF commented Jul 5, 2023

fgravin commented Jul 5, 2023

gkeimeHDF commented Jul 5, 2023 • edited Loading

fgravin commented Jul 5, 2023

fgravin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkeimeHDF Aug 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fgravin Jul 12, 2023 • edited by jahow Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkeimeHDF Aug 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jahow Aug 1, 2023 • edited Loading

Choose a reason for hiding this comment

gkeimeHDF Aug 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkeimeHDF commented Aug 1, 2023

jahow commented Aug 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jahow commented Aug 1, 2023 • edited Loading

gkeimeHDF commented Aug 1, 2023

f-necas commented Aug 1, 2023

gkeimeHDF commented Aug 1, 2023

f-necas commented Aug 2, 2023

jahow commented Aug 25, 2023

gkeimeHDF commented Sep 4, 2023 • edited Loading

fgravin commented Sep 5, 2023

fgravin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkeimeHDF Sep 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jahow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkeimeHDF commented May 30, 2023 •

edited

Loading

gkeimeHDF commented Jul 5, 2023 •

edited

Loading

gkeimeHDF Aug 1, 2023 •

edited

Loading

fgravin Jul 12, 2023 •

edited by jahow

Loading

gkeimeHDF Aug 1, 2023 •

edited

Loading

jahow Aug 1, 2023 •

edited

Loading

gkeimeHDF Aug 1, 2023 •

edited

Loading

jahow commented Aug 1, 2023 •

edited

Loading

gkeimeHDF commented Sep 4, 2023 •

edited

Loading

gkeimeHDF Sep 11, 2023 •

edited

Loading

gkeimeHDF commented Oct 18, 2023 •

edited

Loading

gkeimeHDF commented Oct 18, 2023 •

edited

Loading

fgravin commented Oct 19, 2023 •

edited

Loading