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 an API form generator for OGC API Records #687

Merged
merged 11 commits into from
Dec 22, 2023

Conversation

cmoinier
Copy link
Collaborator

@cmoinier cmoinier commented Nov 13, 2023

This PR creates an API form component to handle customization of OGC API urls.

It also adds a date-range picker component to gn-ui (because such an input was in the original mockup, and will be used in gn-editor in the future).

Funded by Métropole de Lille

image

@cmoinier cmoinier marked this pull request as draft November 13, 2023 14:45
Copy link
Contributor

github-actions bot commented Nov 13, 2023

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

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

Copy link
Member

@fgravin fgravin left a comment

Choose a reason for hiding this comment

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

Hi @cmoinier, not a real review but some thoughts from here to there ☺️

@fgravin fgravin changed the title dh/custom api Datahub: add an API form generator for OGC API Records Nov 15, 2023
@fgravin fgravin added enhancement Proposal for a new feature minor feature labels Nov 15, 2023
@fgravin fgravin modified the milestones: 2.0.2, 2.1.0 Nov 15, 2023
Copy link
Member

@fgravin fgravin left a comment

Choose a reason for hiding this comment

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

Thanks @cmoinier for your progress on this.
I've made some general remarks regarding code practice.
Let me know if it make sense for you ☺️

@cmoinier cmoinier marked this pull request as ready for review December 6, 2023 14:30
@coveralls
Copy link

coveralls commented Dec 6, 2023

Coverage Status

coverage: 83.069% (+0.09%) from 82.98%
when pulling ee4ffb6 on DH/vue-api-datahub
into c9e78c3 on main.

@fgravin
Copy link
Member

fgravin commented Dec 6, 2023

Could we see a screenshot ?? ☺️

@cmoinier cmoinier force-pushed the DH/vue-api-datahub branch 4 times, most recently from dfa4d2b to 72dff1c Compare December 7, 2023 15:07
Copy link
Collaborator

@tkohr tkohr left a comment

Choose a reason for hiding this comment

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

Great work @cmoinier ! Thanks for your continued effort on this. I've left some minor comments. Besides that, I still see two points to address:

  • Currently, the form breaks on mobile. I guess we can just hide the three dots button on mobile or replace it with the copy url button. Not sure if this has been discussed.
  • Comparing the form with the mockup I still see quite some differences, like font sizes, paddings and margins, line breaks of the labels.

Copy link
Member

@fgravin fgravin left a comment

Choose a reason for hiding this comment

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

Very good work, you've added a bunch of e2e & unit tests so it's very complete 👏

Just yes like @tkohr says, there are many slight differences between the mockup and your design (fonts, sizes, margin etc..). Please try to replicate the mockup with accuracy, it's all about details 😄

Thanks

@fgravin fgravin changed the title Datahub: add an API form generator for OGC API Records [MEL] [Datahub] Add an API form generator for OGC API Records Dec 12, 2023
@fgravin fgravin linked an issue Dec 12, 2023 that may be closed by this pull request
@cmoinier
Copy link
Collaborator Author

Thanks @tkohr @fgravin for your reviews. I have corrected most of the things you mentionned as per the last commit. I still need to improve the design before it's considered done. 🙂

@fgravin
Copy link
Member

fgravin commented Dec 14, 2023

Thanks @tkohr @fgravin for your reviews. I have corrected most of the things you mentionned as per the last commit. I still need to improve the design before it's considered done. 🙂

Thanks @cmoinier we will have a look.
BTW I am wondering how do you test ? I think that having a metadata which references an OGC API Features resources in the support-services compo would help.
I don't know how hard it is though.

Thanks

@cmoinier
Copy link
Collaborator Author

cmoinier commented Dec 14, 2023

@fgravin I added a dump with a metadata that has two OGC API Features resources (so we can also test switching between cards).

@fgravin fgravin changed the title [MEL] [Datahub] Add an API form generator for OGC API Records [Datahub] Add an API form generator for OGC API Records Dec 14, 2023
Copy link
Member

@fgravin fgravin left a comment

Choose a reason for hiding this comment

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

Still some issues with the rendering.
You don't handle the width of your form correctly, see the "preview" container which behave the same.

  • on full screen, the form is too large
  • on mobile, it's too narrow.

Also, the style of the copy text input is not like the mockup.

@fgravin
Copy link
Member

fgravin commented Dec 17, 2023

On my tests, the cross of the close button is bigger than the text.
image

@fgravin
Copy link
Member

fgravin commented Dec 17, 2023

Using the support-services locally, the dataset "Accroche velo" with the ogc api features has lost the "similar records" panel.
Is it related to the PR ?

@cmoinier
Copy link
Collaborator Author

cmoinier commented Dec 18, 2023

Noted for the rendering issues, I'm on it.

For the "similar records" panel, yes, it is related to the PR. I think I have accidentally deleted the similar record link and therefore, the panel isn't displaying anymore. I will add it again and push another dump.

EDIT : After checking on main, it seems that this particular record never had any similar records in the first place. I added an OGC API service to the record /dataset/ee965118-2416-4d48-b07e-bbc696f002c2 and the similar record section stays on, so it is unrelated.

@fgravin fgravin removed this from the 2.1.0 milestone Dec 20, 2023
@cmoinier cmoinier force-pushed the DH/vue-api-datahub branch 2 times, most recently from b151b01 to 0f6bfbf Compare December 21, 2023 17:09
Copy link
Collaborator

@tkohr tkohr left a comment

Choose a reason for hiding this comment

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

Thanks, @cmoinier, everything looks good on my side! I just added a last commit to add some opacity to the transition and remove some unnecessary code, I still came across.

I let you have a look at it and wait for the CI to pass, before merging :-)

@cmoinier cmoinier dismissed fgravin’s stale review December 22, 2023 09:55

Changes done & reviewed by tobias

@cmoinier cmoinier merged commit 80787c2 into main Dec 22, 2023
8 checks passed
@cmoinier cmoinier deleted the DH/vue-api-datahub branch December 22, 2023 09:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Proposal for a new feature minor feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Datahub] Add OGC API Features url generator
4 participants