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

Me/record field temporal extents #876

Merged
merged 7 commits into from
Jun 14, 2024

Conversation

LHBruneton-C2C
Copy link
Collaborator

@LHBruneton-C2C LHBruneton-C2C commented May 16, 2024

Description

This PR introduces:

  • the sortable list element
  • the date picker input

The date range picker input has been made fully functional.

Both date picker and date range picker are full width, to adapt to the parent design.

The record form field now supports temporal extents.

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

@LHBruneton-C2C LHBruneton-C2C requested a review from jahow May 16, 2024 13:45
Copy link
Contributor

github-actions bot commented May 16, 2024

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

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

Copy link
Contributor

github-actions bot commented May 16, 2024

📷 Screenshots are here!

@LHBruneton-C2C LHBruneton-C2C force-pushed the ME/record-field-temporal-extents branch from 5774cc6 to b768937 Compare May 23, 2024 13:18
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.

Thank you, impressive work already 🙂

For the SortableList component I was wondering if instead of using a Componentoutput we could instead use a simpler TemplateOutput. Then inside the template provided to the SortableList, a distinction could be made to either show a range picker or a date picker.

In the InteractiveTable component for instance, the cells are rendered using a template, and inside the template we can decide whether to show, text, buttons, etc.

See

<ng-template #cell let-item>
<div
class="flex justify-start items-center gap-2"
*ngIf="getRecordFormats(item) as formats"
[title]="formats.join(', ')"
>
<span
class="badge-btn min-w-[45px] text-sm text-white px-2 shrink-0"
[style.background-color]="getBadgeColor(formats[0])"
*ngIf="formats[0]"
>
{{ formats[0] }}
</span>
<span
class="badge-btn min-w-[45px] text-sm text-white px-2 shrink-0"
[style.background-color]="getBadgeColor(formats[1])"
*ngIf="formats[1]"
>
{{ formats[1] }}
</span>
<div class="shrink-0" *ngIf="formats.slice(2).length > 0">
<span>+{{ formats.slice(2).length }}</span>
</div>
</div>
<div *ngIf="!getRecordFormats(item)"></div>
</ng-template>

Maybe this way we can simply give the array of temporal extents to the sortable list as input, and get the actual extents on its output "elementsChange" events? I'm not 100% sure but it might make the FormFieldTemporalExtent component simpler.

@LHBruneton-C2C
Copy link
Collaborator Author

@jahow Thanks for the pre-review, and the template suggestion. I don't see how to make it work with user input though. Here, in the table, you're just displaying data. But in the Sortable list, the elements can be added/reordered/deleted, but their content can be changed too. That's where I found the ComponentOutlet most useful, as it can take an input (and an output, by making the input subscribable).

@LHBruneton-C2C LHBruneton-C2C force-pushed the ME/record-field-temporal-extents branch from b768937 to 8d4a909 Compare May 30, 2024 13:54
@LHBruneton-C2C LHBruneton-C2C marked this pull request as ready for review May 30, 2024 13:54
@LHBruneton-C2C LHBruneton-C2C force-pushed the ME/record-field-temporal-extents branch from 8d4a909 to 2cce192 Compare May 30, 2024 14:38
@LHBruneton-C2C
Copy link
Collaborator Author

@jahow If you didn't before, you can do a full review now. If you already did, I only added some changes with the date picker and date range picker design.

@LHBruneton-C2C LHBruneton-C2C force-pushed the ME/record-field-temporal-extents branch from 2cce192 to 108a0a5 Compare May 31, 2024 07:34
@coveralls
Copy link

coveralls commented May 31, 2024

Coverage Status

coverage: 83.919% (+0.6%) from 83.286%
when pulling 4cdadbb on ME/record-field-temporal-extents
into 3e7f46f on main.

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.

Thank you!! It works well in the editor form. I made a few comments and I think the date and range pickers will still need some work (e.g. the range picker closing after first click, the pickers not showing up when clicking the text, the date format not being localized).

I also feel that the form-field-temporal-extents component ends up being quite complex, either Angular is really making this complicated or there is a simpler way to have the same result that we're missing. Either way, I think we can keep it as it is for now :) and it's definitely not a trivial component anyways.

@@ -13,6 +13,25 @@ export default {
],
} as Meta<DateRangePickerComponent>

export const Primary: StoryObj<DateRangePickerComponent> = {
export const StartEnd: StoryObj<DateRangePickerComponent> = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be nice to also have the output events of this component show up in Storybook :) see for instance https://github.com/geonetwork/geonetwork-ui/blob/main/libs/ui/inputs/src/lib/star-toggle/star-toggle.stories.ts

tailwind.base.css Show resolved Hide resolved
Comment on lines +36 to +43
addOptions$ = combineLatest([
this.translateService
.get('editor.record.form.temporalExtents.addDate')
.pipe(map((buttonLabel) => ({ buttonLabel, eventName: 'date' }))),
this.translateService
.get('editor.record.form.temporalExtents.addRange')
.pipe(map((buttonLabel) => ({ buttonLabel, eventName: 'range' }))),
])
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm still unsure if the "add options" shouldn't just be buttons outside of the SortableList component, that might make this kind of logic not necessary and reduce the responsibility of the SortableList? Not saying we should necessarily rework this now, I'd just be curious to hear your opinion.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I kinda feel that the component responsible for deleting elements should also deal with re-adding them...

@LHBruneton-C2C LHBruneton-C2C force-pushed the ME/record-field-temporal-extents branch from 108a0a5 to 4cdadbb Compare June 6, 2024 14:58
@LHBruneton-C2C LHBruneton-C2C merged commit 6c8dcba into main Jun 14, 2024
9 checks passed
@LHBruneton-C2C LHBruneton-C2C deleted the ME/record-field-temporal-extents branch June 14, 2024 08:26
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