-
Notifications
You must be signed in to change notification settings - Fork 0
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
[Dataset page] Information block #19
Conversation
311d341
to
7e25a44
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 for adding the information block! It looks great already! 🚀
I have some comments, but mainly I noticed some small things in the design.
- For the keywords in the categories I think the row-gap should be a bit bigger (maybe gap-y-2) and then I think the same can be applied to the territory
- I am not sure about the design for the license and producer, there are two versions. One without underline and one with (but for me this looks a bit like a link), also I think in one design the value is on the next line
- the value of the producer is not on the same (baseline) as the key
Let me know if something is not clear or if you need help.
"mel.dataset.download": "Télécharger", | ||
"mel.dataset.favorite": "Favoris", | ||
"mel.dataset.informations": "Informations", | ||
"mel.dataset.less": "Reduire", | ||
"mel.dataset.licenses": "Licence(s) :", |
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.
Is the "(s)" needed? I did not see it in the design.
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.
Good question, if we have plural, we could also use translateParams
, to display either singular or plural. Not sure if all licenses should be listed here, or just the first.
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.
Not sure either, and they can be pretty long strings. I'm going to ask Christophe about it.
apps/datahub/src/app/dataset/dataset-information/dataset-information.component.html
Outdated
Show resolved
Hide resolved
apps/datahub/src/app/dataset/dataset-information/dataset-information.component.html
Show resolved
Hide resolved
apps/datahub/src/app/dataset/dataset-header/dataset-header.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 for the nice component @cmoinier ! I've added a couple more ideas inline.
apps/datahub/src/app/dataset/dataset-information/dataset-information.component.html
Outdated
Show resolved
Hide resolved
apps/datahub/src/app/dataset/dataset-information/dataset-information.component.ts
Outdated
Show resolved
Hide resolved
b67ca84
to
f65bb72
Compare
Thanks @Angi-Kinas and @tkohr for the reviews 🙂 The changes addressed with the last commits are :
|
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.
Nice! Thanks, for all the rework @cmoinier!
Once the fixed width is removed from the component and the last adaptions are made, as discussed everything LGTM 🙂
76a2275
to
b4b19a3
Compare
b4b19a3
to
1b9601d
Compare
This PR adds the information block elements to the dataset page.
What's included
What is not included
Screenshot