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 anchcors scrolling #1150

Merged
merged 1 commit into from
Mar 25, 2024
Merged

Conversation

Koc
Copy link
Contributor

@Koc Koc commented Mar 9, 2024

📝 Summary

Steps to reproduce:

  • Create long page with multiple headings
  • Click on heading in the bottom of the page
  • Copy url from address bar with anchor and open it in new tab

Expected result:

  • Content scrolled to heading

Actual result:

  • There are no scroll

🖼️ Screenshots

🏚️ Before 🏡 After
image image

🚧 TODO

🏁 Checklist

  • Code is properly formatted (npm run lint / npm run stylelint / composer run cs:check)
  • Sign-off message is added to all commits
  • Tests (unit, integration and/or end-to-end) passing and the changes are covered with tests
  • Documentation (README or documentation) has been updated or is not required

@Koc Koc force-pushed the feature/fix-anchcors-scrolling branch from 2889952 to bda1252 Compare March 10, 2024 22:27
@juliusknorr juliusknorr requested a review from mejo- March 11, 2024 05:59
@juliusknorr juliusknorr added bug Something isn't working 3. to review labels Mar 11, 2024
@juliusknorr juliusknorr self-requested a review March 11, 2024 05:59
@mejo- mejo- force-pushed the feature/fix-anchcors-scrolling branch from bda1252 to 1dcd676 Compare March 11, 2024 19:07
@mejo-
Copy link
Member

mejo- commented Mar 14, 2024

For some reason onLoaded callback is not called in my case, so I'm using setTimeout. Any idea how to fix that?

Currently only the session editor (Editor.vue in the text app) supports the onLoaded callback. But you're right, the editor used for the view mode (MarkdownContentEditor in the text app) should do so as well. I opened nextcloud/text#5489 to implement this.

src/mixins/editorMixin.js Outdated Show resolved Hide resolved
src/mixins/editorMixin.js Outdated Show resolved Hide resolved
@Koc
Copy link
Contributor Author

Koc commented Mar 21, 2024

@mejo- I've updated current PR, and opened one more to nextcloud-vue. Please review one more time.

Copy link
Member

@mejo- mejo- left a comment

Choose a reason for hiding this comment

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

Dear @Koc, great to hear that onLoaded works now 😊

Regarding your PR against nextcloud-vue, I think that we should rather change the behaviour here in Collectives. It's not given that the contents given to NcAppContent contain a heading like in collectives. Adding something like the following to Collectives.vue should do the trick:

:deep(.splitpanes__pane-details) {
	scroll-padding-top: 60px;
	scroll-behavior: smooth;
}

Copy link
Contributor

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

Signed-off-by: Konstantin Myakshin <molodchick@gmail.com>
@Koc Koc force-pushed the feature/fix-anchcors-scrolling branch from c45360f to 0847e80 Compare March 25, 2024 08:55
@Koc
Copy link
Contributor Author

Koc commented Mar 25, 2024

@mejo- thank you for suggestion!
I've pushed changes and closed related PR. The only one thing - I've omit :deep to be consistent with other styles. It works fine without it, as non-scopped styles used

Copy link
Member

@mejo- mejo- left a comment

Choose a reason for hiding this comment

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

Awesome, works like a charm now. Thanks a lot for the contribution @Koc!

Only #1114 sometimes breaks the behaviour, but that's a separate issue.

@mejo- mejo- merged commit 01cc927 into nextcloud:main Mar 25, 2024
41 of 43 checks passed
@Koc Koc deleted the feature/fix-anchcors-scrolling branch March 25, 2024 11:02
@Koc Koc restored the feature/fix-anchcors-scrolling branch June 10, 2024 16:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants