Change scroll top button to component #2170

yucheng11122017 · 2023-02-15T14:57:55Z

What is the purpose of this pull request?

Fixes #2081

Overview of changes:
Change scroll to top button to be a Vue component. This is to be inserted into layout file.
This allow users to

Not use it if they want
Change the icon
Change the icon size and color
Change the position

Anything you'd like to highlight/discuss:

Testing instructions:
Insert <scroll-top-button></scroll-top-button> to layout file.

Proposed commit message: (wrap lines at 72 characters)
Change scroll top button to component

It is currently added to all pages, cannot be disabled, and does not support styling or positioning.

This limits the usability and customization for users.

Changing the scroll top button to a component allows users to not include it in their layout if they want and allows them to style it by passing attributes to it.

Let's make

scroll top button no longer compulsory and to be added to layout file if users want it
scroll top button added to default file when project is initialized
support different icons for button
support positioning for scroll top button
support customized icon size and color

Checklist: ☑️

Updated the documentation for feature additions and enhancements
Added tests for bug fixes or features
Linked all related issues
No unrelated changes

Breaking change descriptions:
Scroll to Top button is changed to a component to allow for more flexibility in its usage. This includes allowing users to

Not use it if they want
Change the icon
Change the icon size and color
Change the position

It is no longer automatically added to pages on the website. To add the Scroll to Top Button to a website, add the component to the layout page

In layout page
...
<scroll-top-button />
...

This button is still added to the default layout when users initialise a Markbind project.

…mponentifyBackToTopButton

jovyntls

Thank you for the work @yucheng11122017 ! This is a nice component to have 🎉 The fading in/out is a nice touch!

I'm not sure if I missed it (from all of the changes in the test files oops) - are there tests for this new component?

packages/vue-components/src/ScrollTopButton.vue

docs/userGuide/syntax/scrollTopButton.md

packages/vue-components/src/ScrollTopButton.vue

…mponentifyBackToTopButton

yucheng11122017 · 2023-02-18T02:15:22Z

Thank you for the work @yucheng11122017 ! This is a nice component to have 🎉 The fading in/out is a nice touch!

I'm not sure if I missed it (from all of the changes in the test files oops) - are there tests for this new component?

Hi @jovyntls, thank you for your comments! I have updated them accordingly. I also added test cases for the component.
One thing I am struggling with is testing that the button appears after the user scrolls. I think the problem is that there is a set timeout which only updates the button after 0.1s so the display in the button doesn't update immediately. I was wondering if you would have any advice on how to tackle this problem? :)

jovyntls · 2023-02-19T03:54:07Z

Hi @jovyntls, thank you for your comments! I have updated them accordingly. I also added test cases for the component. One thing I am struggling with is testing that the button appears after the user scrolls. I think the problem is that there is a set timeout which only updates the button after 0.1s so the display in the button doesn't update immediately. I was wondering if you would have any advice on how to tackle this problem? :)

Thanks for looking out for that test case! Hmm might be tricky, will does combining these work?:

Add a scroll event on the test
Using a setTimeout to wait before asserting the button

jovyntls

Thanks for the tests @yucheng11122017 ! Just a few more nits

docs/userGuide/syntax/scrollTopButton.md

packages/vue-components/src/ScrollTopButton.vue

docs/userGuide/syntax/scrollTopButton.md

package-lock.json

tlylt

May I confirm if this means the scroll-top button is no longer a default (i.e. included when someone does markbind init?)

packages/vue-components/src/ScrollTopButton.vue

yucheng11122017 · 2023-03-12T03:47:50Z

@tlylt I added the scroll top button to the default layout in packages/core/template/default/_markbind/layouts/default.md so when markbind init is called, it will still be included.

tlylt · 2023-03-12T04:29:31Z

@tlylt I added the scroll top button to the default layout in packages/core/template/default/_markbind/layouts/default.md so when markbind init is called, it will still be included.

Can we include a line explaining how this can optionally be removed in the docs?

Also, this is indeed breaking change as mentioned in the original issue, will pause the merge until we want to move on to v5

tlylt · 2023-03-12T04:31:19Z

Can we also include all details in the proposed commit message? Will need these in the release draft for this major change.

tlylt · 2023-03-16T09:34:00Z

Hi @yucheng11122017, thank you for the work on this. I think we are ready to merge this soon if you could help resolve the conflicts.

tlylt · 2023-03-16T09:37:21Z

https://se-education.org/guides/conventions/git.html

@yucheng11122017 can you kindly help to adjust the commit message format according to the guide, please? Main reason why I think the format is superior is not just for the sake of consistency, but also because it highlights the rationale behind a change, which is crucial context for future devs.

…mponentifyBackToTopButton # Conflicts: # packages/cli/test/functional/test_site/expected/bugs/index.page-vue-render.js # packages/cli/test/functional/test_site/expected/index.page-vue-render.js # packages/cli/test/functional/test_site/expected/sub_site/index.page-vue-render.js # packages/cli/test/functional/test_site/expected/sub_site/nested_sub_site/index.page-vue-render.js # packages/cli/test/functional/test_site/expected/sub_site/nested_sub_site/testNunjucksPathResolving.page-vue-render.js # packages/cli/test/functional/test_site/expected/sub_site/testNunjucksPathResolving.page-vue-render.js # packages/cli/test/functional/test_site/expected/testAnchorGeneration.page-vue-render.js # packages/cli/test/functional/test_site/expected/testAnnotate.page-vue-render.js # packages/cli/test/functional/test_site/expected/testAntiFOUCStyles.page-vue-render.js # packages/cli/test/functional/test_site/expected/testCenterText.page-vue-render.js # packages/cli/test/functional/test_site/expected/testCodeBlocks.page-vue-render.js # packages/cli/test/functional/test_site/expected/testDates.page-vue-render.js # packages/cli/test/functional/test_site/expected/testEmptyFrontmatter.page-vue-render.js # packages/cli/test/functional/test_site/expected/testExternalScripts.page-vue-render.js # packages/cli/test/functional/test_site/expected/testHr.page-vue-render.js # packages/cli/test/functional/test_site/expected/testImages.page-vue-render.js # packages/cli/test/functional/test_site/expected/testIncludeBoilerplate.page-vue-render.js # packages/cli/test/functional/test_site/expected/testIncludeMultipleModals.page-vue-render.js # packages/cli/test/functional/test_site/expected/testIncludePluginsRendered.page-vue-render.js # packages/cli/test/functional/test_site/expected/testLayouts.page-vue-render.js # packages/cli/test/functional/test_site/expected/testLayoutsOverride.page-vue-render.js # packages/cli/test/functional/test_site/expected/testLinks.page-vue-render.js # packages/cli/test/functional/test_site/expected/testMath.page-vue-render.js # packages/cli/test/functional/test_site/expected/testModals.page-vue-render.js # packages/cli/test/functional/test_site/expected/testNunjucksPathResolving.page-vue-render.js # packages/cli/test/functional/test_site/expected/testPageNav.page-vue-render.js # packages/cli/test/functional/test_site/expected/testPageNavPrint.page-vue-render.js # packages/cli/test/functional/test_site/expected/testPageNavTarget.page-vue-render.js # packages/cli/test/functional/test_site/expected/testPageNavWithOnlyTitle.page-vue-render.js # packages/cli/test/functional/test_site/expected/testPageNavWithoutTitleAndNavHeadings.page-vue-render.js # packages/cli/test/functional/test_site/expected/testPanelMarkdownParsing.page-vue-render.js # packages/cli/test/functional/test_site/expected/testPanels.page-vue-render.js # packages/cli/test/functional/test_site/expected/testPanelsClosingTransition.page-vue-render.js # packages/cli/test/functional/test_site/expected/testPlantUML.page-vue-render.js # packages/cli/test/functional/test_site/expected/testPopoverTrigger.page-vue-render.js # packages/cli/test/functional/test_site/expected/testPopovers.page-vue-render.js # packages/cli/test/functional/test_site/expected/testThumbnails.page-vue-render.js # packages/cli/test/functional/test_site/expected/testTooltipSpacing.page-vue-render.js # packages/cli/test/functional/test_site/expected/testTree.page-vue-render.js # packages/cli/test/functional/test_site/expected/testVariableContainsInclude.page-vue-render.js # packages/cli/test/functional/test_site/expected/test_md_fragment.page-vue-render.js # packages/cli/test/functional/test_site_templates/test_default/expected/contents/topic1.page-vue-render.js # packages/cli/test/functional/test_site_templates/test_default/expected/contents/topic2.page-vue-render.js # packages/cli/test/functional/test_site_templates/test_default/expected/contents/topic3a.page-vue-render.js # packages/cli/test/functional/test_site_templates/test_default/expected/contents/topic3b.page-vue-render.js # packages/cli/test/functional/test_site_templates/test_default/expected/index.page-vue-render.js

tlylt · 2023-03-17T00:49:52Z

Tips on how to fix the conflicts:

ensure the fork is latest
ensure local master is in sync with fork master
checkout to pr branch
git merge master
accept any (just choose one) changes to conflicts in generated test files, doesn't really matter as we will override them
once the merge is done, npm run updatetest to generate latest test files

This reverts commit e93a5c3.

…mponentifyBackToTopButton # Conflicts: # packages/cli/test/functional/test_site/expected/bugs/index.page-vue-render.js # packages/cli/test/functional/test_site/expected/index.page-vue-render.js # packages/cli/test/functional/test_site/expected/sub_site/index.page-vue-render.js # packages/cli/test/functional/test_site/expected/sub_site/nested_sub_site/index.page-vue-render.js # packages/cli/test/functional/test_site/expected/sub_site/nested_sub_site/testNunjucksPathResolving.page-vue-render.js # packages/cli/test/functional/test_site/expected/sub_site/testNunjucksPathResolving.page-vue-render.js # packages/cli/test/functional/test_site/expected/testAnchorGeneration.page-vue-render.js # packages/cli/test/functional/test_site/expected/testAnnotate.page-vue-render.js # packages/cli/test/functional/test_site/expected/testAntiFOUCStyles.page-vue-render.js # packages/cli/test/functional/test_site/expected/testCenterText.page-vue-render.js # packages/cli/test/functional/test_site/expected/testCodeBlocks.page-vue-render.js # packages/cli/test/functional/test_site/expected/testDates.page-vue-render.js # packages/cli/test/functional/test_site/expected/testEmptyFrontmatter.page-vue-render.js # packages/cli/test/functional/test_site/expected/testExternalScripts.page-vue-render.js # packages/cli/test/functional/test_site/expected/testHr.page-vue-render.js # packages/cli/test/functional/test_site/expected/testImages.page-vue-render.js # packages/cli/test/functional/test_site/expected/testIncludeBoilerplate.page-vue-render.js # packages/cli/test/functional/test_site/expected/testIncludeMultipleModals.page-vue-render.js # packages/cli/test/functional/test_site/expected/testIncludePluginsRendered.page-vue-render.js # packages/cli/test/functional/test_site/expected/testLayouts.page-vue-render.js # packages/cli/test/functional/test_site/expected/testLayoutsOverride.page-vue-render.js # packages/cli/test/functional/test_site/expected/testLinks.page-vue-render.js # packages/cli/test/functional/test_site/expected/testMath.page-vue-render.js # packages/cli/test/functional/test_site/expected/testModals.page-vue-render.js # packages/cli/test/functional/test_site/expected/testNunjucksPathResolving.page-vue-render.js # packages/cli/test/functional/test_site/expected/testPageNav.page-vue-render.js # packages/cli/test/functional/test_site/expected/testPageNavPrint.page-vue-render.js # packages/cli/test/functional/test_site/expected/testPageNavTarget.page-vue-render.js # packages/cli/test/functional/test_site/expected/testPageNavWithOnlyTitle.page-vue-render.js # packages/cli/test/functional/test_site/expected/testPageNavWithoutTitleAndNavHeadings.page-vue-render.js # packages/cli/test/functional/test_site/expected/testPanelMarkdownParsing.page-vue-render.js # packages/cli/test/functional/test_site/expected/testPanels.page-vue-render.js # packages/cli/test/functional/test_site/expected/testPanelsClosingTransition.page-vue-render.js # packages/cli/test/functional/test_site/expected/testPlantUML.page-vue-render.js # packages/cli/test/functional/test_site/expected/testPopoverTrigger.page-vue-render.js # packages/cli/test/functional/test_site/expected/testPopovers.page-vue-render.js # packages/cli/test/functional/test_site/expected/testThumbnails.page-vue-render.js # packages/cli/test/functional/test_site/expected/testTooltipSpacing.page-vue-render.js # packages/cli/test/functional/test_site/expected/testTree.page-vue-render.js # packages/cli/test/functional/test_site/expected/testVariableContainsInclude.page-vue-render.js # packages/cli/test/functional/test_site/expected/test_md_fragment.page-vue-render.js # packages/cli/test/functional/test_site_templates/test_default/expected/contents/topic1.page-vue-render.js # packages/cli/test/functional/test_site_templates/test_default/expected/contents/topic2.page-vue-render.js # packages/cli/test/functional/test_site_templates/test_default/expected/contents/topic3a.page-vue-render.js # packages/cli/test/functional/test_site_templates/test_default/expected/contents/topic3b.page-vue-render.js # packages/cli/test/functional/test_site_templates/test_default/expected/index.page-vue-render.js

yucheng11122017 · 2023-03-17T03:11:18Z

Thank you for the advice @tlylt! I managed to solve the merge conflicts

Change scroll top button to component It is currently added to all pages, cannot be disabled, and does not support styling or positioning. This limits the usability and customization for users. Changing the scroll top button to a component allows users to not include it in their layout if they want and allows them to style it by passing attributes to it. Let's make - scroll top button no longer compulsory and to be added to layout file if users want it - scroll top button added to default file when project is initialized - support different icons for button - support positioning for scroll top button - support customized icon size and color

yucheng11122017 · 2023-04-02T10:53:57Z

Tips on how to fix the conflicts:

ensure the fork is latest

ensure local master is in sync with fork master

checkout to pr branch

git merge master

accept any (just choose one) changes to conflicts in generated test files, doesn't really matter as we will override them

once the merge is done, npm run updatetest to generate latest test files

Hi @tlylt, I was thinking about adding these instructions about solving merge conflicts to the DG which talks about updating functional test. Might be helpful for other devs!

tlylt · 2023-04-02T11:17:12Z

Tips on how to fix the conflicts:

ensure the fork is latest

ensure local master is in sync with fork master

checkout to pr branch

git merge master

accept any (just choose one) changes to conflicts in generated test files, doesn't really matter as we will override them

once the merge is done, npm run updatetest to generate latest test files

Hi @tlylt, I was thinking about adding these instructions about solving merge conflicts to the DG which talks about updating functional test. Might be helpful for other devs!

Sounds good 👍

yucheng11122017 added 16 commits February 6, 2023 17:16

[WIP] Comment off making ScrollToTopButton default

4f3113c

Merge branch 'master' of https://github.com/MarkBind/markbind into co…

3d0fe19

…mponentifyBackToTopButton

Merge branch 'master' of https://github.com/MarkBind/markbind into co…

5371181

…mponentifyBackToTopButton

Make scrollToTopButton not default

74d5d1e

Add scrollToButton component

4ff635b

Add scrollToTopButton to default and minimal layout

b0002f8

Add support for customising icon, icon color and icon size

afba3e1

Add support for customised positioning

3e08094

Add documentation for scrollToTopButton

702cf7a

Remove unecessary div

31d4ee2

Add scrollToTopButton to documentation

8bf04a5

Update test cases

b1354cf

Delete scrollTopButton.js

6c2e81a

Fix wrong backslash

4ed8ce8

Rename scroll-to-top-button to scroll-top-button

45c5084

Change scrollToTopButton to scrollTopButton

ae7c309

jovyntls suggested changes Feb 16, 2023

View reviewed changes

yucheng11122017 added 7 commits February 17, 2023 14:24

Move CSS styles into scrollTopButton

2589c23

Refactor code to use tenary statements

2da7c94

Add test cases for scrollTopButton

68bc5d5

Change iconSize in scrollTopButton to be the same as iconSize in box

e202741

Merge branch 'master' of https://github.com/MarkBind/markbind into co…

f1b5e5c

…mponentifyBackToTopButton

Fix formatting for style in scrollTopButton

6297c58

Change to use class instead of id for css style

d3fda20

jovyntls reviewed Feb 19, 2023

View reviewed changes

docs/userGuide/syntax/scrollTopButton.md Outdated Show resolved Hide resolved

packages/vue-components/src/ScrollTopButton.vue Outdated Show resolved Hide resolved

docs/userGuide/syntax/scrollTopButton.md Outdated Show resolved Hide resolved

yucheng11122017 added 3 commits February 20, 2023 15:08

Change icon to use slots

7b2b8ee

Update UG to be consistent with box

43c7716

Update test cases

b9cc694

tlylt reviewed Mar 12, 2023

View reviewed changes

package-lock.json Outdated Show resolved Hide resolved

tlylt requested changes Mar 12, 2023

View reviewed changes

packages/vue-components/src/ScrollTopButton.vue Show resolved Hide resolved

yucheng11122017 added 2 commits March 12, 2023 11:10

Restore package-lock.json

b8e76c7

Fix indentation

7072690

tlylt modified the milestones: v4.2.0, v5.0.0 Mar 12, 2023

tlylt added the s.OnHold label Mar 12, 2023

jovyntls added the breakingChange 💥 Feature will behave significantly different, or is made obsolete label Mar 12, 2023

Add instructions on removing scroll top button to UG

e93a5c3

tlylt removed the s.OnHold label Mar 16, 2023

yucheng11122017 added 2 commits March 16, 2023 19:58

Update test cases

f1fa775

yucheng11122017 added 4 commits March 17, 2023 10:23

Revert "Add instructions on removing scroll top button to UG"

5e67fe4

This reverts commit e93a5c3.

Update test cases

d4b24d5

tlylt approved these changes Mar 17, 2023

View reviewed changes

tlylt merged commit 2cebe9e into MarkBind:master Mar 17, 2023

yucheng11122017 mentioned this pull request Apr 3, 2023

Add tip on solving merge conflicts in functional tests #2261

Merged

11 tasks

tlylt mentioned this pull request Apr 29, 2023

Should CollapseExpandButtons component be built into site-nav? #2299

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change scroll top button to component #2170

Change scroll top button to component #2170

yucheng11122017 commented Feb 15, 2023 •

edited

Loading

jovyntls left a comment

yucheng11122017 commented Feb 18, 2023

jovyntls commented Feb 19, 2023

jovyntls left a comment

tlylt left a comment

yucheng11122017 commented Mar 12, 2023

tlylt commented Mar 12, 2023

tlylt commented Mar 12, 2023

tlylt commented Mar 16, 2023

tlylt commented Mar 16, 2023

tlylt commented Mar 17, 2023 •

edited

Loading

yucheng11122017 commented Mar 17, 2023

yucheng11122017 commented Apr 2, 2023

tlylt commented Apr 2, 2023

Change scroll top button to component #2170

Change scroll top button to component #2170

Conversation

yucheng11122017 commented Feb 15, 2023 • edited Loading

jovyntls left a comment

Choose a reason for hiding this comment

yucheng11122017 commented Feb 18, 2023

jovyntls commented Feb 19, 2023

jovyntls left a comment

Choose a reason for hiding this comment

tlylt left a comment

Choose a reason for hiding this comment

yucheng11122017 commented Mar 12, 2023

tlylt commented Mar 12, 2023

tlylt commented Mar 12, 2023

tlylt commented Mar 16, 2023

tlylt commented Mar 16, 2023

tlylt commented Mar 17, 2023 • edited Loading

yucheng11122017 commented Mar 17, 2023

yucheng11122017 commented Apr 2, 2023

tlylt commented Apr 2, 2023

yucheng11122017 commented Feb 15, 2023 •

edited

Loading

tlylt commented Mar 17, 2023 •

edited

Loading