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

feat(xudt-tag): add xudt tag column in xudt page #378

Merged
merged 21 commits into from
Jul 12, 2024
Merged

Conversation

Copy link

vercel bot commented Jun 13, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
ckb-explorer-frontend-in-magickbase-repo ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 11, 2024 3:45am

@Keith-CY
Copy link
Member

Is this PR ready for review?

@Daryl-L
Copy link
Author

Daryl-L commented Jun 18, 2024

Is this PR ready for review?

Not yet, just waiting for the API ready

src/components/MultiFilterButton/index.tsx Outdated Show resolved Hide resolved
src/components/MultiFilterButton/index.tsx Outdated Show resolved Hide resolved
src/components/MultiFilterButton/index.tsx Outdated Show resolved Hide resolved
src/components/MultiFilterButton/index.tsx Outdated Show resolved Hide resolved
src/pages/Xudts/index.tsx Outdated Show resolved Hide resolved
const params = useSearchParams(filterName)
const filter = params[filterName]
let types = filteredList.map(f => f.value)
if (filter !== undefined) {
Copy link
Member

Choose a reason for hiding this comment

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

It can be simplified as

const types = params[filterName]?.split(',').filter(t => !t)

Copy link
Member

Choose a reason for hiding this comment

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

filterName should be filterNames if it's designed to be multiple filters.

Copy link
Member

Choose a reason for hiding this comment

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

For me, the filters from the URL are more like filteredList because they are effective/working filters while filteredList in the component params is a list of filters.

Copy link
Author

Choose a reason for hiding this comment

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

filterName should be filterNames if it's designed to be multiple filters.

I think there would be only one filter for each filter button, so I think "s" is not necessary here.

Copy link
Author

Choose a reason for hiding this comment

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

There are three values for the filterName.

  1. undefined, which means to entry the page without any filter parameters, for "all selected".
  2. Empty string '', like tags= which means "not selected any value".
  3. Partial selected.

Copy link
Member

Choose a reason for hiding this comment

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

There are three values for the filterName.

  1. undefined, which means to entry the page without any filter parameters, for "all selected".
  2. Empty string '', like tags= which means "not selected any value".
  3. Partial selected.
const types = params[filterName]?.split(',').filter(t => !t)

works well for these 3 cases

when params[filterName] is undefined, types is undefined, means all selected
when params[filterName] is an empty string, types is an empty array, means not selected any value
when params[filterName] is a string, types is expected to be an array with items, means some selected

Copy link
Member

Choose a reason for hiding this comment

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

Any update on this conversation?

Copy link
Author

Choose a reason for hiding this comment

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

filterName should be filterNames if it's designed to be multiple filters.

I think there would be only one filter for each filter button, so I think "s" is not necessary here.

The logic is as follows

const filter = params[filterName]
const types = filter.split(',').filter(t => t !== '')

equivalent to

const types = params[filterName].split(',').filter(t => t !== '')

So params[filterName] is expected to be a string like A,B,C, which includes multiple values. That's the reason I think it should be filterNames

The filterName means the name of the filter, and A,B,C should be the values of this filter, not the name.

Copy link
Author

Choose a reason for hiding this comment

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

There are three values for the filterName.

  1. undefined, which means to entry the page without any filter parameters, for "all selected".
  2. Empty string '', like tags= which means "not selected any value".
  3. Partial selected.
const types = params[filterName]?.split(',').filter(t => !t)

works well for these 3 cases

when params[filterName] is undefined, types is undefined, means all selected when params[filterName] is an empty string, types is an empty array, means not selected any value when params[filterName] is a string, types is expected to be an array with items, means some selected

The result of !t is true when it is an empty string, but the empty string should filtered to the result

Copy link
Member

Choose a reason for hiding this comment

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

My typo, .filter(t => !t) should be .filter(t => !!t) to return truty values

src/components/MultiFilterButton/index.tsx Outdated Show resolved Hide resolved
src/components/MultiFilterButton/index.tsx Outdated Show resolved Hide resolved
src/components/MultiFilterButton/index.tsx Outdated Show resolved Hide resolved
@Keith-CY
Copy link
Member

Keith-CY commented Jul 2, 2024

@Sven-TBD please review the UX

@Sven-TBD
Copy link

Sven-TBD commented Jul 2, 2024

I'm ok with the UX part, however, while trying to uncheck all , it should be No data, but all items are displayed here.
image
@Daryl-L

@Daryl-L
Copy link
Author

Daryl-L commented Jul 2, 2024

I'm ok with the UX part, however, while trying to uncheck all , it should be No data, but all items are displayed here.

image

@Daryl-L

Should be supported by the API @zmcNotafraid How to filter none tag items?

@Daryl-L
Copy link
Author

Daryl-L commented Jul 9, 2024

https://ckb-explorer-frontend-in-magickbase-repo-iubv4tkve-magickbase.vercel.app/xudts

1、I found that the data obtained by Select and Verified on platform/Category are the same. Do they need to be distinguished? @Sven-TBD

Request URL: https://ckb-explorer-api-staging.magickbase.com/api/v1/xudts?page=1&page_size=10&sort=transactions.desc&tags=invalid,suspicious,out-of-length-range,duplicate,layer-1-asset,layer-2-asset,verified-on,supply-limited,supply-unlimited,rgbpp-compatible image

Request URL: https://ckb-explorer-api-staging.magickbase.com/api/v1/xudts?page=1&page_size=10&sort=transactions.desc&tags=invalid,suspicious,out-of-length-range,duplicate,layer-1-asset,layer-2-asset,supply-limited,supply-unlimited,rgbpp-compatible,category image

2、Cannot get data when there is no Verified on platform or Category.

image image image
3、It is OK to use it alone, but data cannot be obtained when using Invalid and Suspicious together. Similar issues include out of length range and duplicate or more tags.

https://ckb-explorer-frontend-in-magickbase-repo-iubv4tkve-magickbase.vercel.app/xudts?page=5 https://ckb-explorer-frontend-in-magickbase-repo-iubv4tkve-magickbase.vercel.app/xudts?page=5&tags=suspicious Expected https://ckb-explorer-frontend-in-magickbase-repo-iubv4tkve-magickbase.vercel.app/xudts?page=1&tags=suspicious

4、When browsing to page 5 in the Select state, if there are multiple pages of data when selecting different tags later, the data needs to be displayed from the home page. (When selecting Suspicious alone, it needs to display page=1&tags=suspicious, but now it directly displays page=5&tags=suspicious.)

parameter union added

@FrederLu
Copy link

FrederLu commented Jul 9, 2024

Magickbase/ckb-explorer-public-issues#650 (comment)
image
image

1、The corresponding backend message information does not have the Verified on platform/xudt.tag.category information. Is the unname tag added? cc @Sven-TBD


4、When browsing to page 5 in the Select state, if there are multiple pages of data when selecting different tags later, the data needs to be displayed from the home page. (When selecting Suspicious alone, it needs to display page=1&tags=suspicious, but now it directly displays page=5&tags=suspicious.)

2、The problem still exists.


image

3、When there is no data on the page, paging needs to be performed here. At this time, if you switch pages again, the pages will be without data.


image image

4、 There is Invalid data, but the data cannot be obtained through Request URL: https://ckb-explorer-api-staging.magickbase.com/api/v1/xudts?page=10&page_size=10&sort=transactions.desc&tags=invalid&union=true.


image image

5、If the filter function is used, the icon needs to have a highlighted style, refer to the NFT Collection page.

@Sven-TBD
Copy link

Sven-TBD commented Jul 9, 2024

1、The corresponding backend message information does not have the Verified on platform/xudt.tag.category information. Is the unname tag added?

Yes, We can still remain the tag, Verified on platform. But we need to remove the xudt.tag.category @Daryl-L @zmcNotafraid

@FrederLu
Copy link

FrederLu commented Jul 10, 2024

1、The corresponding backend message information does not have the Verified on platform/xudt.tag.category information. Is the unname tag added?

Yes, We can still remain the tag, Verified on platform. But we need to remove the xudt.tag.category @Daryl-L @zmcNotafraid

There is also no Verified on platform provided in the API and this also needs to be removed.

@Daryl-L
Copy link
Author

Daryl-L commented Jul 10, 2024

1、The corresponding backend message information does not have the Verified on platform/xudt.tag.category information. Is the unname tag added?
Yes, We can still remain the tag, Verified on platform. But we need to remove the xudt.tag.category @Daryl-L @zmcNotafraid

There is also no Verified on platform provided in the API and this also needs to be removed.

Removed.
By the way, I recommend to manage the tag configs in backend, and write a document to list.

@FrederLu
Copy link

FrederLu commented Jul 10, 2024

3、When there is no data on the page, paging needs to be performed here. At this time, if you switch pages again, the pages will be without data.
5、If the filter function is used, the icon needs to have a highlighted style, refer to the NFT Collection page.

These two issues remain to be resolved.


image

When Tags is empty on the mobile terminal, the page still displays data.

@Daryl-L
Copy link
Author

Daryl-L commented Jul 11, 2024

3、When there is no data on the page, paging needs to be performed here. At this time, if you switch pages again, the pages will be without data.
5、If the filter function is used, the icon needs to have a highlighted style, refer to the NFT Collection page.

These two issues remain to be resolved.

image When Tags is empty on the mobile terminal, the page still displays data.

All solved

@Keith-CY Keith-CY merged commit 57edf66 into develop Jul 12, 2024
8 checks passed
@Keith-CY Keith-CY deleted the feature/xudt-tag branch July 12, 2024 02:25
@Sven-TBD
Copy link

关于Filter组件:

并集API:
“未启用” = { 实际选中项=全选,样式= {不高亮icon+选项全不选}},
“启用” = {实际选中项={用户选项},样式= {高亮icon+选项=实际选中项}};

交集API:
“未启用” = { 实际选中项=全不选,样式= {不高亮icon+选项全不选}},
“启用” = {实际选中项={用户选项},样式= {高亮icon+选项=实际选中项}};

其中默认为未启用状态,未启用状态下,选中一个选项将变更组件状态到 “启用”:
启用-------选项全不选------>未启用

其中启用状态下,选项全不选将变更组件状态到 “未启用”:
未启用 -------选中一项选项------> 启用

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants