Skip to content

feat: Update scrollToBottom and scrollToTop methods to allow smooth behaviour#259

Open
quietdreamr wants to merge 1 commit into
OvidijusParsiunas:mainfrom
quietdreamr:add-scroll-smoothening
Open

feat: Update scrollToBottom and scrollToTop methods to allow smooth behaviour#259
quietdreamr wants to merge 1 commit into
OvidijusParsiunas:mainfrom
quietdreamr:add-scroll-smoothening

Conversation

@quietdreamr
Copy link
Copy Markdown
Contributor

@quietdreamr quietdreamr commented Sep 5, 2024

Uses the scrollTo() method instead of directly setting the scrollTop. By default, it will still scroll instantly. When passing smooth = true, it'll set the behaviour to smooth.

@OvidijusParsiunas
Copy link
Copy Markdown
Owner

Hey @quietdreamr.
Thankyou for your contribution. I am currently quite busy, but before I can merge this I need to test it with other behaviours such as - streaming, media images with eventual loading and other UX aspects.
Apologies for not being able complete this right away as I have just moved countries and don't have much free time.

@quietdreamr
Copy link
Copy Markdown
Contributor Author

Hey @quietdreamr. Thankyou for your contribution. I am currently quite busy, but before I can merge this I need to test it with other behaviours such as - streaming, media images with eventual loading and other UX aspects. Apologies for not being able complete this right away as I have just moved countries and don't have much free time.

No problem at all. It is not urgent. All the best with the moving process!

@OvidijusParsiunas
Copy link
Copy Markdown
Owner

Hey, I just had a look at your code and whilst it is great, there are a few things to note:

  • Whilst you have made a great start, there is work to be done on actually allowing the devs to toggle whether the scrolling behaviour should be smooth or instant. This will involve the establishment of a toggle property/attribute that is not intrusive to the existing Deep Chat's API and at the same time would pave way for implementing new scroll related features in the future. This takes time to get right, hence needs to be carefully thought out. Additionally, there will ofcourse need to be work to update the Deep Chat's code to facilitate this and the documentation as well.
  • The smooth scrolling seems to break the stream UX in the FireFox browser as the chat stops scrolling to the bottom. I wonder if anything can be done to fix this.

@OvidijusParsiunas
Copy link
Copy Markdown
Owner

Given the work mentioned above and outstanding issues that I still need to get through, I will keep the PR open but not merge it until I am comfortable with preparing it for a new release.

(I know that the code does not affect the existing functionality, but because not all of its branches can be activated - I do not want to confuse any other devs that may be using/analysing this project's code).

Thankyou for your great initiative once again!

@OvidijusParsiunas OvidijusParsiunas added the enhancement New feature or request label Sep 7, 2024
@OvidijusParsiunas OvidijusParsiunas self-assigned this Sep 7, 2024
Copy link
Copy Markdown

@JiwaniZakir JiwaniZakir left a comment

Choose a reason for hiding this comment

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

The changes in elementUtils.ts look solid overall — replacing direct scrollTop assignment with scrollTo() is the right approach for enabling smooth scroll behavior. A few observations:

The ternary (smooth ? 'smooth' : 'instant') can be simplified to just passing smooth ? 'smooth' : 'instant' without the extra parentheses, which is a minor style nit. More importantly, behavior: 'instant' has limited browser support compared to the old element.scrollTop = value pattern — it behaves as auto in some browsers, so it's worth verifying the fallback behavior is acceptable across target environments (particularly older Safari versions).

Also worth considering: callers of scrollToBottom that are invoked during streaming or auto-scroll may call this frequently in rapid succession. With smooth: true, overlapping smooth scroll calls can interfere with each other and cause jittery behavior, so any callers enabling smooth scroll should be reviewed to ensure they debounce or only trigger on discrete user-facing actions rather than continuous updates. It may be worth adding a note about this in the method's JSDoc or in the PR description to guide future callers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants