fix(DH): Make font-sizes and colors same across the expandable panel #660

Angi-Kinas · 2023-10-24T08:32:20Z

Changed the font-size and color as proposed to:

Also made opening of the panel faster.

github-actions · 2023-10-24T08:33:50Z

Affected libs: ui-elements, feature-catalog, feature-record, feature-router, feature-search, feature-map, feature-dataviz, ui-catalog, ui-search, ui-layout,
Affected apps: metadata-editor, datahub, demo, webcomponents, search, map-viewer,

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

coveralls · 2023-10-24T08:41:59Z

coverage: 86.546% (+4.7%) from 81.812% when pulling f6c3fe5 on DH-fix-bug-font-size into f87f421 on main.

fgravin · 2023-10-24T08:48:40Z

Thanks @Angi-Kinas
Some remarks regarding the mockup:

1. D’où viennent ces données ?

Margin between labels and values are too big
The text, before the grid is not black

2. Licence ou conditions d’utilisation ?

Text should be small
Would be good to have the label stuff, if possible. Meaning we should be able to categorize the licence into a label.

3. Détails
The right margin after the icon seems too big

Overall, fonts are 14px & 16px, but it's hard to understand strictly the mockup as the font is completely different 😄

tkohr · 2023-10-24T09:10:11Z

Thanks @Angi-Kinas ! I'm wondering if the font sizes are not ok in this PR though @fgravin...

1. D’où viennent ces données ?
* The text, before the grid is not black

I think this will be handled by the theming config, no?

2. Licence ou conditions d’utilisation ?
* Text should be small

Lucile lately proposed in a JIRA comment to put it in primary color (and then same size as primary text in other panels, I guess). It would look more consistent if there was also a label inside the panel I guess. But primary in 14px won't look consistent neither I think.

jahow

All good on my side, thanks!

tkohr · 2023-10-24T11:54:04Z

libs/ui/elements/src/lib/metadata-info/metadata-info.component.html

@@ -32,27 +35,30 @@
  *ngIf="metadata.lineage"
  [title]="'record.metadata.origin' | translate"
 >
-  <p class="mb-5 pt-4 whitespace-pre-line break-words" gnUiLinkify>
+  <p
+    class="text-gray-900 mb-5 pt-4 whitespace-pre-line break-words"


I thought, we would let the config set the main color here (as well as in the abstract above), but maybe I'm wrong (cc @fgravin @jahow)

fgravin · 2023-10-24T11:54:12Z

libs/ui/elements/src/lib/metadata-info/metadata-info.component.html

@@ -4,11 +4,12 @@
 >
  record.metadata.about
 </p>
-<div class="mb-6 md-description sm:mb-4 sm:pr-16" *ngIf="metadata.abstract">
+<div class="text-gray-900 mb-6 md-description sm:mb-4 sm:pr-16">


The text should be the main text color, I think it means that no color class is required.
We should not use text-gray things I guess.

The text should be the main text color, I think it means that no color class is required. We should not use text-gray things I guess.

the gray-900 is derived from the main color, so that should not be a problem. the main color is almost black and here we wanted a lighter color (as you also mentioned in your review).
cc @tkohr

Ok, I thought using the main color would allow more consistence with other components, but looking at the card on the home page we also use the derived text-gray-900, so it seems ok to me, as long as we try to stay consistent with the derived grays ;-)

Why do you remove *ngIf="metadata.abstract" ? Is it the abstract container or not ?

And yes, the abstract should have the main color, and not text-gray-900 I guess.

I think the colors are:

Abstract title: black
Abstract content: main

D’où viennent ces données ? title: black
Text underneath: main
Property label: grayer
Property color: primary

The ngIf was not removed but just moved to the paragraph. Otherwise the content ghost will not be shown.

Regarding the colors:
main is almost black, so compared to the mockups it would be too dark.

Maybe we shouldn't use pure black but main because pure black is apparently bad practice and it's not dependent on the theme.

tkohr · 2023-10-24T11:54:50Z

libs/ui/elements/src/lib/metadata-info/metadata-info.component.html

    {{ metadata.lineage }}
  </p>
  <div
    class="py-4 px-6 rounded bg-gray-100 grid grid-cols-2 gap-y-[10px] gap-x-[20px] text-gray-700 info-grid"
  >
    <div *ngIf="metadata.recordUpdated">
      <p class="text-sm" translate>record.metadata.updatedOn</p>
-      <p class="text-primary font-medium mt-2">
+      <p class="text-primary font-medium mt-[4px]">


mt-1 should be the same I think

I think it is more explixit to put it in px, because it reflects what is in the mockup. In terms of generated css it's almost the same.

Ok, so far we've privileged the non interpolated values (also for the lightest CSS possible, I think). I don't have a strong opinion on this, but maybe we should agree on a best practice.

Angi-Kinas · 2023-10-24T11:56:52Z

@fgravin and @tkohr Thanks for the review.
@fgravin I added most of your suggestions (see screenshot).

To point 2:

As Lucile mentioned in the ticket that she wanted to change the way this is being displayed so it will be aligned with the other panels (in blue).
the badge is only set if the flag is true. It would be too hacky to retrieve it from the text as it is right now.

Also:
Olivia and I discovered that there was another bug. The content ghost for the abstract was not shown. We fixed this as well.

fix(DH): Make font-sizes and colors same across the expandable panel

152f6c6

fix(DH): Add suggested changes

bf058f0

jahow approved these changes Oct 24, 2023

View reviewed changes

tkohr reviewed Oct 24, 2023

View reviewed changes

fgravin reviewed Oct 24, 2023

View reviewed changes

tkohr reviewed Oct 24, 2023

View reviewed changes

fix(DH): Make sure that the content ghost for abstract is shown

f6c3fe5

Angi-Kinas force-pushed the DH-fix-bug-font-size branch from e37b98f to f6c3fe5 Compare October 24, 2023 12:04

Angi-Kinas merged commit f8b9e53 into main Oct 24, 2023
7 checks passed

Angi-Kinas deleted the DH-fix-bug-font-size branch October 24, 2023 12:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(DH): Make font-sizes and colors same across the expandable panel #660

fix(DH): Make font-sizes and colors same across the expandable panel #660

Angi-Kinas commented Oct 24, 2023 •

edited

Loading

github-actions bot commented Oct 24, 2023 •

edited

Loading

coveralls commented Oct 24, 2023 •

edited

Loading

fgravin commented Oct 24, 2023

tkohr commented Oct 24, 2023

jahow left a comment

tkohr Oct 24, 2023

fgravin Oct 24, 2023

Angi-Kinas Oct 24, 2023

tkohr Oct 24, 2023

fgravin Oct 24, 2023

Angi-Kinas Oct 24, 2023

tkohr Oct 24, 2023

Angi-Kinas Oct 24, 2023

tkohr Oct 24, 2023

Angi-Kinas commented Oct 24, 2023

fix(DH): Make font-sizes and colors same across the expandable panel #660

fix(DH): Make font-sizes and colors same across the expandable panel #660

Conversation

Angi-Kinas commented Oct 24, 2023 • edited Loading

github-actions bot commented Oct 24, 2023 • edited Loading

coveralls commented Oct 24, 2023 • edited Loading

fgravin commented Oct 24, 2023

tkohr commented Oct 24, 2023

jahow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Angi-Kinas commented Oct 24, 2023

Angi-Kinas commented Oct 24, 2023 •

edited

Loading

github-actions bot commented Oct 24, 2023 •

edited

Loading

coveralls commented Oct 24, 2023 •

edited

Loading