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.

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

[TAS-2441] ✨ Implement collector memo section #2002

Merged
merged 20 commits into from
Jan 10, 2025

Conversation

AuroraHuang22
Copy link
Contributor

default.mp4

Copy link

AuroraHuang22 added a commit that referenced this pull request Dec 24, 2024
v-if="authorReplied"
:class="[
'absolute bottom-0 right-[6px]',
authorReplied ? 'group-hover:right-[22px]' : '',
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
authorReplied ? 'group-hover:right-[22px]' : '',
{ 'group-hover:right-[22px]': authorReplied },

Copy link
Contributor Author

Choose a reason for hiding this comment

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

]"
:style="bgStyle"
>
<p class="block text-[16px] line-clamp-4">{{ buyerMessage }}</p>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<p class="block text-[16px] line-clamp-4">{{ buyerMessage }}</p>
<p class="block text-[16px] line-clamp-4" v-text="buyerMessage" />

Copy link
Contributor Author

Choose a reason for hiding this comment

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

buyerMessage() {
return this.message?.buyerMessage;
},
authorReplied() {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
authorReplied() {
hasAuthorReplied() {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment on lines 529 to 530
isHistoryInfoLoading: false,
isFinishedLoadingHistory: false,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
isHistoryInfoLoading: false,
isFinishedLoadingHistory: false,
isHistoryInfoLoading: false,
hasHistoryInfoLoaded: false,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

src/util/ui.js Outdated
Comment on lines 25 to 37
export function ellipsisCollectorAddress(value) {
if (value) {
const len = value.length;
const dots = '...';
if (!value) return '';
if (value.length > 10) {
return value.substring(0, 8) + dots;
}
return value;
}
return value;
}

Copy link
Member

Choose a reason for hiding this comment

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

Can reuse this instead?

export function ellipsis(value) {
if (value) {
const len = value.length;
const dots = '...';
if (!value) return '';
if (value.length > 20) {
return value.substring(0, 8) + dots + value.substring(len - 6, len);
}
return value;
}
return value;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nwingt
Copy link
Member

nwingt commented Dec 27, 2024

the resolution of avatar images are too low in retina display, please use 2x or even 3x of the current size
image

Comment on lines 49 to 51
durationTime() {
return this.animationDuration;
},
Copy link
Member

Choose a reason for hiding this comment

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

maybe add unit here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment on lines 77 to 84
.animate-scroll {
animation: scroll var(--animation-duration, 60s) infinite linear;
}

.animate-scroll.delay {
animation: scroll var(--animation-duration, 60s) infinite linear;
animation-delay: calc(var(--animation-duration, 60s) * -1);
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
.animate-scroll {
animation: scroll var(--animation-duration, 60s) infinite linear;
}
.animate-scroll.delay {
animation: scroll var(--animation-duration, 60s) infinite linear;
animation-delay: calc(var(--animation-duration, 60s) * -1);
}
.animate-scroll,
.animate-scroll.delay {
animation: scroll var(--animation-duration, 60s) infinite linear;
}
.animate-scroll.delay {
animation-delay: calc(var(--animation-duration, 60s) * -1);
}

Copy link
Contributor Author

@AuroraHuang22 AuroraHuang22 Dec 30, 2024

Choose a reason for hiding this comment

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

props: {
animationDuration: {
type: String,
default: '60s',
Copy link
Member

Choose a reason for hiding this comment

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

Should we add unit here or just number?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AuroraHuang22 AuroraHuang22 marked this pull request as ready for review December 30, 2024 05:25
AuroraHuang22 added a commit that referenced this pull request Dec 30, 2024
AuroraHuang22 added a commit that referenced this pull request Jan 8, 2025
AuroraHuang22 added a commit that referenced this pull request Jan 8, 2025
AuroraHuang22 added a commit that referenced this pull request Jan 9, 2025
Comment on lines 33 to 35
<p class="text-like-green hover:underline" align="center">
{{ formattedUserDisplayNameFull }}
</p>
Copy link
Member

Choose a reason for hiding this comment

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

align="center" should not have effect in <p/>

Suggested change
<p class="text-like-green hover:underline" align="center">
{{ formattedUserDisplayNameFull }}
</p>
<p
class="text-like-green text-center hover:underline"
v-text="formattedUserDisplayNameFull"
/>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment on lines 52 to 81
<svg
v-if="hasAuthorReplied"
:class="[
'absolute bottom-0 right-[6px]',
{ 'group-hover:right-[22px]': hasAuthorReplied },
]"
width="20"
height="20"
viewBox="0 0 20 20"
fill="none"
xmlns="http://www.w3.org/2000/svg"
>
<circle cx="10" cy="10" r="9.5" fill="#F7F7F7" stroke="#E1E1E1" />
<g opacity="0.5" clip-path="url(#clip0_2298_5781)">
<path
d="M5.2658 8.70773L8.70338 5.73928C9.00428 5.47941 9.47851 5.69037 9.47851 6.09404V7.65758C12.6158 7.69349 15.1035 8.32226 15.1035 11.2954C15.1035 12.4954 14.3304 13.6843 13.4759 14.3058C13.2093 14.4998 12.8292 14.2563 12.9275 13.9419C13.8132 11.1097 12.5075 10.3578 9.47851 10.3142V12.0312C9.47851 12.4355 9.0039 12.6456 8.70338 12.386L5.2658 9.41726C5.04957 9.23051 5.04927 8.89474 5.2658 8.70773Z"
fill="#9B9B9B"
/>
</g>
<defs>
<clipPath id="clip0_2298_5781">
<rect
width="10"
height="10"
fill="white"
transform="translate(5.10352 5)"
/>
</clipPath>
</defs>
</svg>
Copy link
Member

Choose a reason for hiding this comment

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

I think this SVG can be simplified, try union the paths or make a compound path

I wont recommend to use <clipPath/> unless it is necessary as id is problematic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment on lines 195 to 232
<div v-else>
<div
class="flex items-center justify-between mb-[24px] px-[16px] laptop:px-0 sm:px-[32px]"
>
<h3
class="text-[28px] font-600 text-center text-like-green"
v-text="$t('nft_collector_message_label')"
/>
<ButtonV2
preset="tertiary"
size="mini"
:text="$t('nft_collector_message_view_all')"
@click="handleClickMoreCollectorMessage"
/>
</div>
<NFTPageCollectorMessageList
class="transform -translate-x-1/2 w-[100vw] left-1/2"
:class-id="classId"
:creator-avatar="creatorAvatar"
:messages="filterCollectorsWithReplies"
:scroll-speed="7.5"
/>
<div
:class="[
'relative',
'flex',
'justify-between',
'h-[180px] laptop:h-[204px]',
'z-50',
'-mx-[12px] laptop:-mx-[24px] desktop:-mx-[calc((100vw-962px)/2)]',
'-mt-[180px] laptop:-mt-[204px]',
'pointer-events-none',
]"
>
<div :class="[overlayClasses, 'bg-gradient-to-r']" />
<div :class="[overlayClasses, 'bg-gradient-to-l']" />
</div>
</div>
Copy link
Member

Choose a reason for hiding this comment

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

Can use <template/> here, no need for extra div

Suggested change
<div v-else>
<div
class="flex items-center justify-between mb-[24px] px-[16px] laptop:px-0 sm:px-[32px]"
>
<h3
class="text-[28px] font-600 text-center text-like-green"
v-text="$t('nft_collector_message_label')"
/>
<ButtonV2
preset="tertiary"
size="mini"
:text="$t('nft_collector_message_view_all')"
@click="handleClickMoreCollectorMessage"
/>
</div>
<NFTPageCollectorMessageList
class="transform -translate-x-1/2 w-[100vw] left-1/2"
:class-id="classId"
:creator-avatar="creatorAvatar"
:messages="filterCollectorsWithReplies"
:scroll-speed="7.5"
/>
<div
:class="[
'relative',
'flex',
'justify-between',
'h-[180px] laptop:h-[204px]',
'z-50',
'-mx-[12px] laptop:-mx-[24px] desktop:-mx-[calc((100vw-962px)/2)]',
'-mt-[180px] laptop:-mt-[204px]',
'pointer-events-none',
]"
>
<div :class="[overlayClasses, 'bg-gradient-to-r']" />
<div :class="[overlayClasses, 'bg-gradient-to-l']" />
</div>
</div>
<template v-else>
<div
class="flex items-center justify-between mb-[24px] px-[16px] laptop:px-0 sm:px-[32px]"
>
<h3
class="text-[28px] font-600 text-center text-like-green"
v-text="$t('nft_collector_message_label')"
/>
<ButtonV2
preset="tertiary"
size="mini"
:text="$t('nft_collector_message_view_all')"
@click="handleClickMoreCollectorMessage"
/>
</div>
<NFTPageCollectorMessageList
class="transform -translate-x-1/2 w-[100vw] left-1/2"
:class-id="classId"
:creator-avatar="creatorAvatar"
:messages="filterCollectorsWithReplies"
:scroll-speed="7.5"
/>
<div
:class="[
'relative',
'flex',
'justify-between',
'h-[180px] laptop:h-[204px]',
'z-50',
'-mx-[12px] laptop:-mx-[24px] desktop:-mx-[calc((100vw-962px)/2)]',
'-mt-[180px] laptop:-mt-[204px]',
'pointer-events-none',
]"
>
<div :class="[overlayClasses, 'bg-gradient-to-r']" />
<div :class="[overlayClasses, 'bg-gradient-to-l']" />
</div>
</template>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AuroraHuang22 added a commit that referenced this pull request Jan 9, 2025
AuroraHuang22 added a commit that referenced this pull request Jan 9, 2025
@AuroraHuang22 AuroraHuang22 requested a review from nwingt January 9, 2025 09:50
if (!a.buyerMessage && b.buyerMessage) {
return 1;
}
return b.authorReply - a.authorReply;
Copy link
Member

Choose a reason for hiding this comment

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

Is authorReply a string? What is the expected sorting here?

computed: {
displayMessages() {
if (this.messages.length < 3) return [];
let loopedMessages = [...this.messages];
Copy link
Member

Choose a reason for hiding this comment

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

Seems we don't changes this array, we don't need to clone it in that case

@williamchong williamchong merged commit 7f87ac7 into likecoin:develop Jan 10, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants