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

fix(core): set scroll boundary to nearest #6310

Merged
merged 3 commits into from
Apr 29, 2024
Merged

fix(core): set scroll boundary to nearest #6310

merged 3 commits into from
Apr 29, 2024

Conversation

ninaandal
Copy link
Contributor

@ninaandal ninaandal commented Apr 10, 2024

Description

There was an issue with deleting characters in a pte where the length of the paragraph exceeded the size of the box - it would jump to the top of the pte.

By setting the block in the scrollIntoView() to nearest instead of start, which means the browser will align leafEl with the least amount of vertical movement.

Screen.Recording.2024-04-10.at.14.37.46.mov

Fixes 5648

What to review

Deleting characters will not cause the view to jump to the top.

Testing

Notes for release

Fixes issue where the cursor would move out of viewpoint when deleting a character in PTE.
Does changing this have any other implications in the studio?

@ninaandal ninaandal requested review from skogsmaskin, hermanwikner and a team April 10, 2024 12:39
Copy link

vercel bot commented Apr 10, 2024

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

Name Status Preview Comments Updated (UTC)
performance-studio ✅ Ready (Inspect) Visit Preview Apr 29, 2024 9:44am
test-next-studio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 29, 2024 9:44am
test-studio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 29, 2024 9:44am
1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
studio-workshop ⬜️ Ignored (Inspect) Visit Preview Apr 29, 2024 9:44am

Copy link
Contributor

No changes to documentation

Copy link
Contributor

github-actions bot commented Apr 10, 2024

Component Testing Report Updated Apr 29, 2024 9:49 AM (UTC)

File Status Duration Passed Skipped Failed
comments/CommentInput.spec.tsx ✅ Passed (Inspect) 36s 15 0 0
formBuilder/ArrayInput.spec.tsx ✅ Passed (Inspect) 7s 3 0 0
formBuilder/inputs/PortableText/Annotations.spec.tsx ✅ Passed (Inspect) 28s 6 0 0
formBuilder/inputs/PortableText/copyPaste/CopyPaste.spec.tsx ✅ Passed (Inspect) 13s 4 2 0
formBuilder/inputs/PortableText/Decorators.spec.tsx ✅ Passed (Inspect) 14s 6 0 0
formBuilder/inputs/PortableText/FocusTracking.spec.tsx ✅ Passed (Inspect) 36s 15 0 0
formBuilder/inputs/PortableText/Input.spec.tsx ✅ Passed (Inspect) 1m 7s 15 0 0
formBuilder/inputs/PortableText/ObjectBlock.spec.tsx ✅ Passed (Inspect) 1m 6s 18 0 0
formBuilder/inputs/PortableText/RangeDecoration.spec.tsx ✅ Passed (Inspect) 21s 9 0 0
formBuilder/inputs/PortableText/Styles.spec.tsx ✅ Passed (Inspect) 15s 6 0 0
formBuilder/inputs/PortableText/Toolbar.spec.tsx ✅ Passed (Inspect) 23s 9 0 0

@skogsmaskin
Copy link
Member

@ninaandal - I'm currently looking a bit into the underlaying code here as I think it's trying to scroll a bit too aggressively (we have problems elsewhere with this too).

I still need some time to review this.

Copy link
Member

@skogsmaskin skogsmaskin left a comment

Choose a reason for hiding this comment

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

Test this and it really improves the experience.

The jumping @hermanwikner is talking about I will address in another PR.

@skogsmaskin skogsmaskin self-requested a review April 17, 2024 14:06
Copy link
Member

@skogsmaskin skogsmaskin left a comment

Choose a reason for hiding this comment

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

Found an issue. The 'smooth' scrolling should not be there.

Try switching to and from fullscreen mode, and you'll see the issue when focused deep down in a long document.

@RitaDias
Copy link
Contributor

Found an issue. The 'smooth' scrolling should not be there.

Try switching to and from fullscreen mode, and you'll see the issue when focused deep down in a long document.

Good catch @skogsmaskin !
I've pushed a new commit where it removes the smooth completely from the scroll into view which feels like I might just be missing a use case. You can try it out here

Let me know if I'm missing anything 👍

@skogsmaskin
Copy link
Member

Resolved conflicts and rebased this against next.

Copy link
Member

@skogsmaskin skogsmaskin left a comment

Choose a reason for hiding this comment

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

LGTM!

@ninaandal ninaandal added this pull request to the merge queue Apr 29, 2024
Merged via the queue into next with commit 0dae0da Apr 29, 2024
39 checks passed
@ninaandal ninaandal deleted the edx-1080 branch April 29, 2024 10:41
jordanl17 pushed a commit that referenced this pull request Apr 29, 2024
* fix(core): set scroll boundary to nearest

* fix(core): add smooth scroll behavior

* fix(sanity): remove smooth behavior

---------

Co-authored-by: RitaDias <[email protected]>
github-merge-queue bot added a commit that referenced this pull request May 2, 2024
* fix(pte): show toolbar as active only if all blocks have tool active

* fix(pte): show toolbar as active only if all blocks have tool active

* fix(pte): correct toggling of marks when part mark is selected

* fix(core): change scrollintoview block to be nearest (#6328)

* fix(core): set scroll boundary to nearest (#6310)

* fix(core): set scroll boundary to nearest

* fix(core): add smooth scroll behavior

* fix(sanity): remove smooth behavior

---------

Co-authored-by: RitaDias <[email protected]>

* fix(@sanity): issue where hidden unicode characters were bloating document in PTE (#6440)

* fix(portable-text-editor): issue shown in tests re stega. use duplicate code

* test(playwright-ct): add test

* chore(sanity): remove prettier linting

* test(sanity): fix missing snapshot

* test(sanity): update test after realising the test would pass always if comparing object number

* chore: test unicode removal

* chore: test unicode removal

* chore(@sanity): remove old solution

* fix(@sanity/block-tools): unicode issue. remove vercel/stega and move to block-tools

* test(@sanity/block-tools): for unicode

* fix(@sanity/block-tools): utf8 characters weren't beign filtered. using the vercel/stega

* chore: update lock file

* (chore): update pnpm lock

* chore: add codeowners to block-tools

* chore(deps): dedupe pnpm-lock.yaml (#6508)

Co-authored-by: github-merge-queue[bot] <118344674+github-merge-queue[bot]@users.noreply.github.com>

* fix(pte): tidying implementation

* fix(@sanity): issue where hidden unicode characters were bloating document in PTE (#6440)

* fix(portable-text-editor): issue shown in tests re stega. use duplicate code

* test(playwright-ct): add test

* chore(sanity): remove prettier linting

* test(sanity): fix missing snapshot

* test(sanity): update test after realising the test would pass always if comparing object number

* chore: test unicode removal

* chore: test unicode removal

* chore(@sanity): remove old solution

* fix(@sanity/block-tools): unicode issue. remove vercel/stega and move to block-tools

* test(@sanity/block-tools): for unicode

* fix(@sanity/block-tools): utf8 characters weren't beign filtered. using the vercel/stega

* chore: update lock file

* (chore): update pnpm lock

* chore(deps): dedupe pnpm-lock.yaml (#6508)

Co-authored-by: github-merge-queue[bot] <118344674+github-merge-queue[bot]@users.noreply.github.com>

* chore(deps): update dependency @sanity/pkg-utils to v6.8.8 (#6509)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* feat: use prefersLatestPublished parameter in DocumentPaneProvider (#6486)

* fix(@sanity): issue where hidden unicode characters were bloating document in PTE (#6440)

* fix(portable-text-editor): issue shown in tests re stega. use duplicate code

* test(playwright-ct): add test

* chore(sanity): remove prettier linting

* test(sanity): fix missing snapshot

* test(sanity): update test after realising the test would pass always if comparing object number

* chore: test unicode removal

* chore: test unicode removal

* chore(@sanity): remove old solution

* fix(@sanity/block-tools): unicode issue. remove vercel/stega and move to block-tools

* test(@sanity/block-tools): for unicode

* fix(@sanity/block-tools): utf8 characters weren't beign filtered. using the vercel/stega

* chore: update lock file

* (chore): update pnpm lock

* chore(deps): dedupe pnpm-lock.yaml (#6508)

Co-authored-by: github-merge-queue[bot] <118344674+github-merge-queue[bot]@users.noreply.github.com>

* chore(deps): update dependency @sanity/pkg-utils to v6.8.8 (#6509)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* fix(pte): simplifying activeAnnotations logic

* fix(pte): new isAnnotationActive static method

* fix(pte): retoring activeAnnotations method in editor

* fix(pte): retoring activeAnnotations method in editor

* fix(pte): fixing issue with multiple annotations in a single block

* fix(pte): fixing issue with multiple annotations in a single block

* fix(pte): fixing typing issue with PortableTextSpan

* fix(pte): fixing failing test since mark toggling has changed

* fix(pte): improving usage of isTextBlock for createWithPortableTextLists

* fix(portable-text-editor): fix issue where decoration would not target correctly (#6532)

* fix(pte): remove unused function in createWithPortableTextMarkModel

* exploration

* fix(pte): reverting incorrect merge

* fix(pte): testing new logic for tools in PTE

* fix(pte): reorg test cases

---------

Co-authored-by: Nina Andal Aarvik <[email protected]>
Co-authored-by: RitaDias <[email protected]>
Co-authored-by: RitaDias <[email protected]>
Co-authored-by: Per-Kristian Nordnes <[email protected]>
Co-authored-by: ecospark[bot] <128108030+ecospark[bot]@users.noreply.github.com>
Co-authored-by: github-merge-queue[bot] <118344674+github-merge-queue[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: cngonzalez <[email protected]>
cngonzalez pushed a commit that referenced this pull request May 8, 2024
* fix(core): set scroll boundary to nearest

* fix(core): add smooth scroll behavior

* fix(sanity): remove smooth behavior

---------

Co-authored-by: RitaDias <[email protected]>
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.

Rich text: Scroll position jumps when deleting character when paragraph exceeds height of box
4 participants