-
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] Rework the metadata content page #575
Conversation
Affected libs:
|
I think it might be easier to see the changes if there is a screenshot. Could you add one of this section? :) |
6799aa2
to
5a95c73
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.
Well done :) ! It looks really nice on the screen record. And thanks for including that into the PR!
I added some comments, mostly things that I don't know about. We can also have a call about it.
libs/feature/record/src/lib/record-metadata/record-metadata.component.html
Show resolved
Hide resolved
libs/feature/record/src/lib/record-metadata/record-metadata.component.ts
Show resolved
Hide resolved
libs/ui/elements/src/lib/metadata-contact/metadata-contact.component.html
Show resolved
Hide resolved
libs/ui/elements/src/lib/metadata-contact/metadata-contact.component.html
Show resolved
Hide resolved
5a95c73
to
07d6479
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.
LGTM! :-)
Thanks for addressing my comments! I'm leaving you another comment, but that's optional.
libs/feature/record/src/lib/record-metadata/record-metadata.component.ts
Outdated
Show resolved
Hide resolved
302bdc6
to
15d9958
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 @f-necas
I compared to the mockup and many things remain different:
- height/ratio of the image
- button hover background
- top margin of the 'contact' label
- the contact information font size is too big
- the contact logo too big
- the "contact" label, should it be "owner" instead ? idk
I am really not a big fan of the status badge, and I don't think that other people would want it, it's geocat specific. BTW the text is not vertically aligned. I will talked with Lucile and customers about that.
Thanks for the PR though, it's just like we should not tweak too much the metadata presentation for the sake of geocat, to me all of this just bring noise into the view... anyway
<div class="relative"> | ||
<gn-ui-button | ||
class="absolute bottom-2 right-2" | ||
[type]="'outline'" |
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.
probably wrong type compare to the mockup (should be secondary bg).
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.
Out of our 'default' buttons. In this case, is it better to follow the mockup or to implement one of our button ?
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.
You are right, and I think we need more convention about button styling. We should not have so many different button types...
Anyway, I think that gnui-btn
should be replaced by a TW component, keep it like this we'll talk about that later, thanks.
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.
we can simply add a new button type, like outline-secondary
. I think it was the right choice to use the gn-ui-button
component, our strategy is to rely as much as possible on reusable low-level presentation components to avoid having too many TW classes in the HTML.
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.
I do prefer
<button class="btn-secondary">
over <gnui-button type="secondary">
, which just manages CSS.
It might be a matter of taste, but TW component is
- no code
- no maintenance
- easily overridden in the app
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.
You can do the same with pure css, don't you ?
It was my call by talking about "TW components".
Maybe we should keep the gnui-button
and handle the css with TW component within the component CSS instead of from the javascript.
It's not a priority though...
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.
As far as I know, you can't for @apply
.
And for sure, you cannot do nested classes, which is quite useful.
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.
This is what I tested in button.component.css
.btn-text-white {
@apply text-white hover:text-primary-lighter;
}
.btn-text-white-a {
@apply btn-text-white;
}
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.
Strange, we must have something else which preprocess our css ?
https://caniuse.com/sr_css-apply
Not even in mozilla doc : https://developer.mozilla.org/en-US/docs/Web/CSS/At-rule
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.
libs/feature/record/src/lib/record-metadata/record-metadata.component.ts
Show resolved
Hide resolved
Close it, as it is outdated and will be reworked. |
PR is intented to add some UI improvements for records. It adds :
Funded by SwissTopo geocat.ch