-
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: fine tuning #663
Datahub: fine tuning #663
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.
Thanks for making those changes. I just found a minor thing.
libs/ui/layout/src/lib/expandable-panel/expandable-panel.component.html
Outdated
Show resolved
Hide resolved
black when black main when 555 on the geo2france mockup
add a <p> with a margin for consistency sake
remove shadow and border-none
The branch have been rebased and is ready to merge. |
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 fixing all these visual details @fgravin !
libs/ui/catalog/src/lib/organisation-preview/organisation-preview.component.html
Show resolved
Hide resolved
@@ -20,7 +20,7 @@ | |||
> | |||
<div class="mb-3 mt-5 sm:mt-2"> | |||
<div | |||
class="font-title text-21 text-title line-clamp-2 col-start-1 col-span-2 sm:line-clamp-1 group-hover:text-primary" | |||
class="font-title text-21 text-title line-clamp-2 col-start-1 col-span-2 sm:line-clamp-1 group-hover:text-primary transition-colors" |
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.
The transition has not been applied to the feed preview, but I guess this is on purpose (as it already has the shadow transition and primary org title).
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.
yep, I think it was from the mockup, but not sure
See commit messages for more information.
For the color debate, I don't really care. The only thing that I would like is that the geo2france design is respected.
There are way many other inconsistencies in the geo2france page, like too lighter grays all over the place. I don't know what is the best strategy, if there is a contrast issue, we could change the main color for the default theme ? or ask a UI designer to work on a default theme maybe.
The last commit can be removed anyway.