Skip to content
This repository was archived by the owner on Dec 5, 2025. It is now read-only.

Conversation

@zhyd1997
Copy link
Contributor

Thank you for your contribution to the KodaDot NFT gallery.
👇 _ Let's make a quick check before the contribution.

PR type

  • Bugfix
  • Feature
  • Refactoring

What's new?

Before submitting Pull Request, please make sure:

  • My contribution builds clean without any errors or warnings
  • I've merged recent default branch -- main and I've no conflicts
  • I've tried to respect high code quality standards
  • I've didn't break any original functionality
  • I've posted a screenshot of demonstrated change in this PR

Optional

  • I've tested it at </rmrk/collection/26902bc2f7c20c546a-1FVG7>
  • I've tested PR on mobile and everything seems works
  • I found edge cases
  • I've written some unit tests 🧪

Had issue bounty label?

  • Fill up your KSM address: Payout

Community participation

Screenshot

  • My fix has changed sticky header in series and spotlight on UI; a screenshot is best to understand changes for others.
    see the 1st comment I added below

@netlify
Copy link

netlify bot commented Feb 16, 2022

✔️ Deploy Preview for koda-nuxt ready!

🔨 Explore the source changes: 869cedb

🔍 Inspect the deploy log: https://app.netlify.com/sites/koda-nuxt/deploys/620e3191c1cd580007de277d

😎 Browse the preview: https://deploy-preview-2366--koda-nuxt.netlify.app

@zhyd1997
Copy link
Contributor Author

zhyd1997 commented Feb 16, 2022

the docs I followed when finishing the task: Table | Buefy.

👇 have sticky position finished table

page/platform Web Mobile
/series
/spotlight

1. Preview:

series on Web:

Screen.Recording.2022-02-16.at.14.10.48.mov

spotlight on Web:

spotlight_web.mp4

series on Mobile:

Screen.Recording.2022-02-16.at.14.14.02.mov

2. The spotlight on Mobile is hard to implement.

two reasons:

  1. strange behavior caused by tooltip when making :mobile-cards="false" and positioned column name sticky.
Screen.Recording.2022-02-16.at.14.00.53.mov
  1. spotlight id is too long when it is sticky position and covers other columns when horizontal scrolling and mobile-cards=true is better for displaying spotlight details.

@zhyd1997 zhyd1997 marked this pull request as ready for review February 16, 2022 07:03
@yangwao yangwao requested a review from roiLeo February 16, 2022 09:53
@yangwao
Copy link
Member

yangwao commented Feb 16, 2022

oh, I guess for mobile we should stick with the native buefy table component which is already there :)

image

@yangwao
Copy link
Member

yangwao commented Feb 16, 2022

Something is broken there
image

@yangwao yangwao added the S-changes-requested-🤞 PR is almost good to go, just some fine tunning label Feb 16, 2022
@yangwao
Copy link
Member

yangwao commented Feb 16, 2022

Seems same bug for spotlight

image

@yangwao
Copy link
Member

yangwao commented Feb 16, 2022

I guess we can also add more results into the series, like bumping it to 200. As there will be lots of collections coming:)

@zhyd1997
Copy link
Contributor Author

I guess we can also add more results into the series, like bumping it to 200. As there will be lots of collections coming:)

It's breakpoint issue, and maybe you need change your system preference: try set scrolling to always:
system preference

@yangwao
Copy link
Member

yangwao commented Feb 16, 2022

huh, we will ask users to change their settings to use our page? :)

you've posted me showing scrollbars, that's not the issue.
The issue it's not showing the whole content of results and in your PR it seems broken, we need to find out why:)

This is how it should look like, screenshot from https://beta.kodadot.xyz which is main branch :)

image

@zhyd1997
Copy link
Contributor Author

@yangwao the same issue found in https://beta.kodadot.xyz/, there is an empty space left when I resize my window to 25%.

Screen Shot 2022-02-16 at 19 03 46

and we have pagination for it, so more items may take a long time to load.

Copy link
Contributor

@roiLeo roiLeo left a comment

Choose a reason for hiding this comment

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

I would change $table-sticky-header-height size (default is 300px)

@zhyd1997
Copy link
Contributor Author

zhyd1997 commented Feb 16, 2022

I would change $table-sticky-header-height size (default is 300px)

I agree with you, but there is also an outer scrollbar, so we need to scroll back to see what the column name is.

is this the expected behavior?

@yangwao
Copy link
Member

yangwao commented Feb 16, 2022

@yangwao the same issue found in beta.kodadot.xyz, there is an empty space left when I resize my window to 25%.

yes, because you have only 10 items set to show :)

@zhyd1997
Copy link
Contributor Author

zhyd1997 commented Feb 16, 2022

@yangwao Yes, I see 🙈

So any ideas for this issue, just double the table sticky header height (from 300px to 600px)?

@yangwao
Copy link
Member

yangwao commented Feb 16, 2022

@yangwao Yes, I see 🙈

So any ideas for this issue, just double the table sticky header height (from 300px to 600px)?

Probably yes. Let's make it responsive please 🥺

I see that sticky prop is cutting height which is unwanted
https://buefy.org/documentation/table/

@zhyd1997
Copy link
Contributor Author

zhyd1997 commented Feb 16, 2022

@yangwao Yes, it limits the table height to 300px as @roiLeo said.

but if no fixed height (including overwrite the default value) , it's hard to position the table with sticky.

@roiLeo
Copy link
Contributor

roiLeo commented Feb 16, 2022

but if no fixed height (including overwrite the default value) , it's hard to position the table with sticky.

Try this on table header:

top: 120px;
position: sticky;
background-color: #090909;

@zhyd1997
Copy link
Contributor Author

but if no fixed height (including overwrite the default value) , it's hard to position the table with sticky.

Try this on table header:

top: 120px;
position: sticky;
background-color: #090909;

@roiLeo it works well on PC 👍, I will figure out how to do this on mobile.

@yangwao
Copy link
Member

yangwao commented Feb 16, 2022

but if no fixed height (including overwrite the default value) , it's hard to position the table with sticky.

Try this on table header:

top: 120px;
position: sticky;
background-color: #090909;

@roiLeo it works well on PC 👍, I will figure out how to do this on mobile.

on mobile, we don't need sticky header 🥺

@zhyd1997
Copy link
Contributor Author

Sorry for misunderstood.

Screen.Recording.2022-02-17.at.07.57.14.mov

The image is so cute when scrolling, is it allowed?

@zhyd1997
Copy link
Contributor Author

@yangwao
the CI is broken because incompatible node version, I have upgraded node version to 16.14.0 on my local machine, and run lint, it works:
Screen Shot 2022-02-17 at 08 11 18

@zhyd1997
Copy link
Contributor Author

I will update my forked repo (instead of merging the main branch) and submit a new PR to check if CI works.

@zhyd1997
Copy link
Contributor Author

zhyd1997 commented Feb 17, 2022

New PR still failed, I found the reason for it:

The node version is cached by CI (setup-node action), so even if we upgraded it on our codebase, it will still use cached node version 16.13.2

How to fix it?
Set check-latest flag to true

Updated: more simpler and performance related solution:
use specified node version: 16.14.0 instead of 16.

Copy link
Contributor

@roiLeo roiLeo left a comment

Choose a reason for hiding this comment

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

These css rules will be global, they could break other components. You can try to wrap-them in a component class.
Regarding images overlapping table header have you try something to fix it?

@zhyd1997
Copy link
Contributor Author

@roiLeo got it

@yangwao
Copy link
Member

yangwao commented Feb 17, 2022

pay 100 usd

@yangwao yangwao merged commit e9f838e into kodadot:main Feb 17, 2022
@yangwao
Copy link
Member

yangwao commented Feb 17, 2022

😍 Perfect, I’ve sent the payout
💵 $100 @ 163.58 USD/KSM ~ 0.611 $KSM
🧗 H2DifyJqaansQREdGFU4BxZyaApbkCgP1a9Pq4xKaebSFZh
🔗 0xdf8eaeb1b335e02783eaeea11bbe91bb74a9b00001efcdfb22cd50ee0eda6ced

🪅 Let’s grab another issue and get rewarded!
🪄 github.com/kodadot/nft-gallery/issues

@yangwao yangwao added the paid pull-request has been paid label Feb 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

paid pull-request has been paid S-changes-requested-🤞 PR is almost good to go, just some fine tunning

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Sticky header in series and spotlight

3 participants