-
Notifications
You must be signed in to change notification settings - Fork 32
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
Conversation
Affected libs:
|
There was a problem hiding this 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
apps/datahub/src/app/record/custom-api/custom-api.component.html
Outdated
Show resolved
Hide resolved
apps/datahub/src/app/record/custom-api/custom-api.component.html
Outdated
Show resolved
Hide resolved
apps/datahub/src/app/record/custom-api/custom-api.component.html
Outdated
Show resolved
Hide resolved
apps/datahub/src/app/record/record-metadata/record-metadata.component.html
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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
apps/datahub/src/app/record/record-apis/record-apis.component.html
Outdated
Show resolved
Hide resolved
apps/datahub/src/app/record/record-apis/record-apis.component.html
Outdated
Show resolved
Hide resolved
apps/datahub/src/app/record/record-apis/record-apis.component.html
Outdated
Show resolved
Hide resolved
apps/datahub/src/app/record/record-metadata/record-metadata.component.html
Outdated
Show resolved
Hide resolved
apps/datahub/src/app/record/record-metadata/record-metadata.component.html
Outdated
Show resolved
Hide resolved
4e6962b
to
39e6a56
Compare
39e6a56
to
74060a6
Compare
Could we see a screenshot ?? |
dfa4d2b
to
72dff1c
Compare
There was a problem hiding this 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.
apps/datahub/src/app/record/record-metadata/record-metadata.component.html
Outdated
Show resolved
Hide resolved
apps/datahub/src/app/record/record-metadata/record-metadata.component.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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
apps/datahub/src/app/record/record-metadata/record-metadata.component.html
Outdated
Show resolved
Hide resolved
Thanks @cmoinier we will have a look. Thanks |
@fgravin I added a dump with a metadata that has two OGC API Features resources (so we can also test switching between cards). |
There was a problem hiding this 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.
Using the support-services locally, the dataset "Accroche velo" with the ogc api features has lost the "similar records" panel. |
Noted for the rendering issues, I'm on it.
EDIT : After checking on |
c6abc1a
to
2484595
Compare
2484595
to
7f440b6
Compare
b151b01
to
0f6bfbf
Compare
0f6bfbf
to
9675a8b
Compare
There was a problem hiding this 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 :-)
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