-
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
Organisation preview component #284
Organisation preview component #284
Conversation
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.
Thank you, good start.
Some details to be sure the layout has a fixed sized, it should not depend on the content.
The story is not available cause the storybook configuration only include stories in the /lib folder.
Thanks for the work I will take over it now.
@Input() logo: string | ||
@Input() nRecords: number | ||
|
||
constructor() {} |
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.
lint error even appear in there
you forgot npm run lint
:)
changeDetection: ChangeDetectionStrategy.OnPush, | ||
}) | ||
export class OrganizationPreviewDatahubComponent { | ||
@Input() title: string |
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.
It's preferable to create a model for this.
Probably in the shared lib.
@Input() title: string | ||
@Input() description: string | ||
@Input() logo: string | ||
@Input() nRecords: number |
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.
recordCount
instead.
<div | ||
class="border p-3 rounded-lg my-2 w-full h-48 flex justify-center items-center my-3" | ||
> | ||
<img [src]="logo" alt="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.
Should not work does it ?
<img [src]="logo" alt="title" /> | |
<img [src]="logo" [alt]="title" /> |
<div class="px-3 capitalize"> | ||
<span class="font-bold my-2 text-lg"> {{ title }}</span> | ||
<p class="text-gray-800 my-2">{{ description }}</p> | ||
<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.
Remove the button (comment it maybe), the follow and favorite things are not implemented.
moduleMetadata({ | ||
imports: [ | ||
BrowserAnimationsModule, | ||
HttpClientModule, |
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 think it's useless
import { OrganizationPreviewDatahubComponent } from './organization-preview-datahub.component' | ||
import { BrowserAnimationsModule } from '@angular/platform-browser/animations' | ||
|
||
export default { |
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.
your story is not available in storybook
</div> | ||
<div class="px-3 capitalize"> | ||
<span class="font-bold my-2 text-lg"> {{ title }}</span> | ||
<p class="text-gray-800 my-2">{{ description }}</p> |
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 don't see the clamp thing ?
What happens if the text is very long ? Should be cropped
<img [src]="logo" alt="title" /> | ||
</div> | ||
<div class="px-3 capitalize"> | ||
<span class="font-bold my-2 text-lg"> {{ title }}</span> |
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.
Should be cropped if too long
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, just left some comments. Thanks @akhelouat @fgravin !
@Input() title: string | ||
@Input() description: string | ||
@Input() logo: string | ||
@Input() nRecords: number |
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.
Maybe create model and rename nRecords
as suggested earlier ?
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.
Yes but you create an Organisation model in your PR no ?
BrowserAnimationsModule, | ||
HttpClientModule, | ||
MatIconModule, | ||
TranslateModule.forRoot(TRANSLATE_DEFAULT_CONFIG), |
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.
Import UtilSharedModule
as well (I get The pipe 'safe' could not be found in the 'RecordThumbnailComponent'
error)
<div class="text-primary opacity-25 flex leading-6"> | ||
<mat-icon class="text-primary opacity-25 mr-1">folder_open </mat-icon> | ||
<span class="mx-1">{{ nRecords }}</span> | ||
<span translate>record.metadata.publications</span> |
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.
Could be cropped as well to be sure it does not break the design, even though it shouldn't, regarding its expected length.
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 think it's fine, the translation should not be long, and it's meant to be on the same line
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
Added organization preview component GSHDF-550