add julius volz tweet + fix the dots coloring. #852
Conversation
|
I think it looks good. Not sure why it's still a draft. |
nisarhassan12
force-pushed
the
add-new-tweets
branch
from
acd5ad0 to
b0a7377
Compare
nisarhassan12
force-pushed
the
add-new-tweets
branch
from
b0a7377 to
6801e97
Compare
|
Thanks for the review. It was a Draft because that state had issues with dots colouring while using the dots to switch between the tweets there is still an issue see https://github.com/gitpod-io/website/issues/915 I will merge this once that is fixed 🙂 |
|
The dots coloring works all well on mobile but on larges devies i.e desktop and so on it does not work well for the last set of tweets the last dot never gets colored no matter whether you scroll through the scroll bar to the last set of tweets or whether you click on the last dot. In the screenshot below you can see that the scroll bar is at the end and you can't scroll any further but the last dot is not colored.
On mobile it works does not work well i.e the first fot has the same problem that the last dot have on large devices:
I think it is probably due to a logical error or miscalculation of a value. |
| tweetsContainer.addEventListener('scroll', (e) => { | ||
| const currentScrollPosition = tweetsContainer?.scrollLeft | ||
| setCurrentCycle(cycles - Math.floor((tweetsContainer?.scrollWidth - currentScrollPosition) / cycleWidth)) | ||
| const cycle = cycles - Math.ceil((tweetsContainer?.scrollWidth - currentScrollPosition) / scrollBy) | ||
| setDotsColor(cycle) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
I don't feel good about this block of code as it gets re-ran every time the scroll happens i.e I feel like it is quite expensive and unwise to call the setDotsColor function everytime scroll happens would be nice if some how we could detect that the scroll position is at a different set of tweets and then and only then call the set dots color funtion ? i.e only 6 times for 6 sets (each set containing 3 tweets). @jankeromnes Do you think this is something that is worth spending the time for its optimization ? I can try to figure out a solution ?
There was a problem hiding this comment.
Hi @nisarhassan12! As suggested in our 1:1 discussion, please look into:
- How to write high-performance scroll event listeners (i.e. they should never block scrolling, and they should never be a performance problem -- I remember seeing various blog posts about how to do this from people in the front-end community, but I don't have a direct link)
- How to debounce things that happen too frequently (e.g. if this listener happens 1000 times per second, maybe you can debounce it such that it only happens a few times per second)
Additionally, I took a look at why the edge dots are sometimes not selected (last dot on desktop, first on mobile).
The value element.scrollLeft actually goes from 0 to element.scrollWidth - element.clientWidth (taking clientWidth into account is important). So:
- When
scrollLeftis0, the left-most dot should be highlighted - When
scrollLeftisscrollWidth - clientWidth, the right-most dot should be highlighted - When it's in-between, maybe you can divide the percentage by
scrollBy, but first you'll need to verify thatscrollBy * cyclesis the same asscrollWidth
Hope this helps! Let me know if you still run into problems with this (then maybe we could finish solving it together in a call).
Will fix #915 #848