Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 3 commits into from
Oct 24, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
152f6c6
fix(DH): Make font-sizes and colors same across the expandable panel
Angi-Kinas Oct 24, 2023
bf058f0
fix(DH): Add suggested changes
Angi-Kinas Oct 24, 2023
f6c3fe5
fix(DH): Make sure that the content ghost for abstract is shown
Angi-Kinas Oct 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 15 additions & 7 deletions libs/ui/elements/src/lib/metadata-info/metadata-info.component.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ;-)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
image

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

<gn-ui-content-ghost ghostClass="h-32" [showContent]="fieldReady('abstract')">
<p
class="whitespace-pre-line break-words"
[innerHTML]="metadata.abstract | safe: 'html'"
*ngIf="metadata.abstract"
></p>
</gn-ui-content-ghost>
</div>
Expand All @@ -32,27 +33,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"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

gnUiLinkify
>
{{ 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]">
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mt-1 should be the same I think

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{{ metadata.recordUpdated && metadata.recordUpdated.toLocaleString() }}
</p>
</div>
<div *ngIf="metadata.updateFrequency">
<p class="text-sm" translate>record.metadata.updateFrequency</p>
<p class="text-primary font-medium mt-2" translate>
<p class="text-primary font-medium mt-[4px]" translate>
domain.record.updateFrequency.{{ metadata.updateFrequency }}
</p>
</div>
<div *ngIf="metadata.status">
<p class="text-sm" translate>record.metadata.updateStatus</p>
<p class="text-primary font-medium mt-2" translate>
<p class="text-primary font-medium mt-[4px]" translate>
domain.record.status.{{ metadata.status }}
</p>
</div>
Expand All @@ -63,7 +67,11 @@
<gn-ui-badge *ngIf="metadata.extras?.isOpenData">
<span translate>record.metadata.isOpenData</span>
</gn-ui-badge>
<span *ngFor="let usage of usages" gnUiLinkify>
<span
class="text-primary font-medium"
*ngFor="let usage of usages"
gnUiLinkify
>
{{ usage }}
</span>
<span class="noUsage" *ngIf="!hasUsage">
Expand All @@ -79,7 +87,7 @@
<p class="text-sm" translate>record.metadata.sheet</p>
<a [href]="metadata.landingPage" target="_blank">
<mat-icon
class="material-symbols-outlined inline-block align-bottom pt-1.5 text-xs text-black"
class="material-symbols-outlined inline-block align-bottom pt-1.5 text-xs text-black !w-[20px]"
>open_in_new</mat-icon
>
<span class="text-primary break-all">{{ metadata.landingPage }}</span>
Expand Down
4 changes: 2 additions & 2 deletions libs/ui/elements/src/lib/metadata-info/metadata-info.component.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ describe('MetadataInfoComponent', () => {
})
it('should not display the abstract section', () => {
const displayedElement =
fixture.nativeElement.querySelector('.md-description')
fixture.nativeElement.querySelector('.md-description p')
expect(displayedElement).toBeFalsy()
})
})
Expand All @@ -69,7 +69,7 @@ describe('MetadataInfoComponent', () => {
})
it('should display the abstract section', () => {
const displayedElement =
fixture.nativeElement.querySelector('.md-description')
fixture.nativeElement.querySelector('.md-description p')
expect(displayedElement).toBeTruthy()
})
})
Expand Down
5 changes: 1 addition & 4 deletions libs/ui/elements/src/lib/metadata-info/metadata-info.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,7 @@ import {
Input,
Output,
} from '@angular/core'
import {
DatasetDistribution,
DatasetRecord,
} from '@geonetwork-ui/common/domain/record'
import { DatasetRecord } from '@geonetwork-ui/common/domain/record'

@Component({
selector: 'gn-ui-metadata-info',
Expand Down
2 changes: 1 addition & 1 deletion libs/ui/layout/src/lib/expandable-panel/expandable-panel.component.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
</div>
</div>
<div
class="content duration-500 overflow-hidden transition-[max-height]"
class="content duration-250 overflow-hidden transition-[max-height]"
[ngClass]="collapsed ? 'ease-out' : 'ease-in'"
[style.maxHeight]="maxHeight"
#contentDiv
Expand Down
Loading