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.

Sign up for GitHub

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

Jump to bottom

Feature: Add page anchors (inter page links) #2841

Merged
merged 4 commits into from
Sep 1, 2022
Merged

Feature: Add page anchors (inter page links) #2841

merged 4 commits into from
Sep 1, 2022

Conversation

susnux
Copy link
Contributor

@susnux susnux commented Aug 26, 2022

Summary

This is a replacement of #2520 (pushed on this repository to enable the CI correctly).

Most parts were already resolved by #2688 (adding IDs to headings, creating the slugs etc), so this just provides the missing user interface (clickable anchor, copyable link, scrolling into view).

It shows the permalink when hovering the heading:

Showing the permalink on bright mode
Showing the permalink on dark mode

@susnux susnux added enhancement New feature or request design Experience, interaction, interface, … 3. to review javascript labels Aug 26, 2022
@susnux susnux force-pushed the feature/heading-anchors branch from 83f2e23 to 992a6b2 Compare August 26, 2022 12:11
@susnux
Copy link
Contributor Author

susnux commented Aug 26, 2022

/compile amend

@susnux susnux mentioned this pull request Aug 26, 2022
Closed
@vinicius73
Copy link
Member

I just miss some testing coverage.

@susnux
Copy link
Contributor Author

susnux commented Aug 26, 2022 •
edited
Loading

I just miss some testing coverage.

Ok then I will add some tests for it, I guess checking for

  • anchors to be available, and
  • clicking on links scrolls the heading into view

is sufficient?

max-nextcloud
max-nextcloud reviewed Aug 26, 2022
View reviewed changes
Copy link
Collaborator

@max-nextcloud max-nextcloud left a comment

Choose a reason for hiding this comment

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

Looks great. Thanks for taking care of this. Just a minor wording comment.

src/nodes/Heading/HeadingView.vue Outdated Show resolved Hide resolved
@susnux susnux force-pushed the feature/heading-anchors branch 2 times, most recently from 6e45150 to cae5654 Compare August 27, 2022 14:17
@susnux susnux requested a review from max-nextcloud August 27, 2022 14:19
@susnux
Copy link
Contributor Author

susnux commented Aug 27, 2022

Tasks done @vinicius73 @max-nextcloud

@max-nextcloud
Copy link
Collaborator

Wording change looks good. Thanks!

@susnux susnux force-pushed the feature/heading-anchors branch from cae5654 to 56f77b5 Compare August 29, 2022 15:02
@vinicius73
Copy link
Member

/compile

@susnux susnux force-pushed the feature/heading-anchors branch from 840a1e7 to d918bd9 Compare August 30, 2022 13:30
@susnux
Copy link
Contributor Author

susnux commented Aug 30, 2022

/compile amend

@vinicius73
Copy link
Member

It has style issues

image

@susnux
Copy link
Contributor Author

susnux commented Aug 30, 2022

It has style issues

Which browser do you use? (So I can reproduce and try to fix it).

@vinicius73 vinicius73 force-pushed the feature/heading-anchors branch from 257cb86 to ea1678b Compare August 30, 2022 17:42
@vinicius73
Copy link
Member

Which browser do you use? (So I can reproduce and try to fix it).

Chrome 104

I've sent a commit about this, check if it is ok pls.

@susnux susnux force-pushed the feature/heading-anchors branch 2 times, most recently from 8850cae to 34596c0 Compare August 31, 2022 15:23
@susnux
Copy link
Contributor Author

susnux commented Aug 31, 2022

@vinicius73 thank you, now it works perfectly!

@susnux
Copy link
Contributor Author

susnux commented Aug 31, 2022

/compile amend

@susnux susnux force-pushed the feature/heading-anchors branch from 3e0e371 to 34596c0 Compare August 31, 2022 19:52
susnux and others added 3 commits September 1, 2022 09:34
@susnux
Add visible and clickable links for the page anchors
ddc73ff 
Handle inter-page links properly and use a heading view
for displaying links and link anchors. Implements #2173

Also fix link handling, to scroll to selected header when
clicking on an anchor link (`#some-id`).

Signed-off-by: Ferdinand Thiessen <rpm@fthiessen.de>
@susnux
Add tests for heading anchors and inter-page links
359a16d 
Also added a chai assertion for checking an element
is currently shown in the viewport of the window.
This is needed as the cypress visibility checks fail
for tiptap elements, as they are overlaid by the author color / names.

Signed-off-by: Ferdinand Thiessen <rpm@fthiessen.de>
💄 (#299): adjust link position
24543c7 
Signed-off-by: Vinicius Reis <vinicius.reis@nextcloud.com>
@vinicius73 vinicius73 force-pushed the feature/heading-anchors branch from 34596c0 to 24543c7 Compare September 1, 2022 12:34
@vinicius73
Copy link
Member

/compile

@nextcloud-command
Compile assets
4c87a0f 
Signed-off-by: nextcloud-command <nextcloud-command@users.noreply.github.com>
@vinicius73 vinicius73 merged commit 0f2c48e into master Sep 1, 2022
@delete-merged-branch delete-merged-branch bot deleted the feature/heading-anchors branch September 1, 2022 16:24
@blizzz blizzz mentioned this pull request Sep 1, 2022
Merged
@juliusknorr
Copy link
Member

Very nice. Thanks again for all the great contributions @susnux

In case you haven't seen we will have the Nextcloud conference taking place in about a moth again in Berlin with a lot of talks over the weekend but also some coding days afterwards and it would be quite cool to have you there and meet each other in person. There is also travel support for contributors. More info is available at http://nextcloud.com/conference-2022/ or if you have any questions just let me know.

@vinicius73 vinicius73 mentioned this pull request Sep 6, 2022
Closed
vinicius73 pushed a commit that referenced this pull request Sep 8, 2022
⏪️ Revert "Merge pull request #2841 from nextcloud/feature/heading-an…
998aa72 
…chors" ultil #2868 be fixed

This reverts commit 0f2c48e, reversing
changes made to 9973d1f.
@vinicius73 vinicius73 mentioned this pull request Sep 8, 2022
Merged
vinicius73 pushed a commit that referenced this pull request Sep 8, 2022
⏪️ Revert "Merge pull request #2841 from nextcloud/feature/heading-an…
c363e46 
…chors" until #2868 be fixed

This reverts commit 0f2c48e, reversing
changes made to 9973d1f.

Signed-off-by: Vinicius Reis <vinicius.reis@nextcloud.com>
vinicius73 pushed a commit that referenced this pull request Sep 9, 2022
⏪️ Revert "Merge pull request #2841 from nextcloud/feature/heading-an…
f59361c 
…chors" until #2868 be fixed

This reverts commit 0f2c48e, reversing
changes made to 9973d1f.

Signed-off-by: Vinicius Reis <vinicius.reis@nextcloud.com>
vinicius73 pushed a commit that referenced this pull request Sep 9, 2022
Merge pull request #2877 from nextcloud/revert/2841
2a02d3a 
⏪️ Revert #2841 (page anchors)
@susnux
Copy link
Contributor Author

susnux commented Oct 3, 2022

@juliushaertl I would loved to, but besides being invited for a important familiar event, I am down with the flu so it is and was not possible for me participate. Maybe in the future!

@juliusknorr juliusknorr mentioned this pull request Oct 11, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vinicius73 vinicius73 vinicius73 approved these changes

@juliusknorr juliusknorr Awaiting requested review from juliusknorr

@max-nextcloud max-nextcloud Awaiting requested review from max-nextcloud

Assignees
No one assigned
Labels
3. to review design Experience, interaction, interface, … enhancement New feature or request javascript
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Feature-request: inter-page links Links to sections?
5 participants
@susnux @vinicius73 @max-nextcloud @juliusknorr @nextcloud-command