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

Virtual scroll code tidying #969

Merged
merged 10 commits into from
Jun 2, 2020
Merged

Virtual scroll code tidying #969

merged 10 commits into from
Jun 2, 2020

Conversation

chrismclarke
Copy link
Member

PR Type

  • Code Tidying

PR Checklist

  • - Latest master branch merged
  • - PR title descriptive (can be used in release notes)
  • - Passes Tests

Description

Following #962 (thanks @tudi2d!), some minor code tidying

  • swap getter function with regular (as it's used in multiple places it runs the code multiple times)
  • simplify row/column calculation methods
  • add overscanrowcount property (change default lookahead from 10 rows to 2)

Screenshots/Videos

Probably deserves one as really neat how images now only load in as they scrolled past!

@cypress
Copy link

cypress bot commented Jun 1, 2020



Test summary

60 0 0 0


Run details

Project onearmy-community-platform
Status Passed
Commit 8262953 ℹ️
Started Jun 1, 2020 9:45 PM
Ended Jun 1, 2020 9:49 PM
Duration 04:30 💡
OS Linux Ubuntu Linux - 16.04
Browser Multiple

View run in Cypress Dashboard ➡️


This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

Copy link
Contributor

@BenGamma BenGamma left a comment

Choose a reason for hiding this comment

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

Logic wise looks good to me, thanks for the little code tyding @chrismclarke !

One little thing though is the number of howto cards per row. On my screen on desktop I have 4 cards (see screenshot), when we should have at maximum 3 howTo cards in one row, can you have a look at this ?

Capture d’écran 2020-06-01 à 11 58 09

rowHeight={410}
overscanRowCount={2}
rowRenderer={data =>
Copy link
Contributor

Choose a reason for hiding this comment

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

Here is there a way we use the onResize prop to update the width of the filteredHowToRow ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, yeah I've actually done that locally but forgot to push. I might add a couple more commits and re-request

@chrismclarke
Copy link
Member Author

chrismclarke commented Jun 1, 2020

Ok, so updated:

  • Moved to own component to (hopefully) make more reusable in future and leave howto code tidier
  • Add method to recalculate on window resize
  • Fix max columns (now 3)
  • Auto calculate heights based on row content
  • Add typings

@chrismclarke chrismclarke requested a review from BenGamma June 1, 2020 21:54
@BenGamma BenGamma merged commit 2a7ac99 into master Jun 2, 2020
@BenGamma BenGamma deleted the 493/virtual-scroll branch June 2, 2020 09:18
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.

2 participants