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

Harmonize and fix back buttons behavior and inconsistent scroll behaviour #102

Merged
merged 4 commits into from
Mar 12, 2024

Conversation

githyuvi
Copy link
Contributor

@githyuvi githyuvi commented Mar 4, 2024

Fixes #72
Issue 1 - Clicking browser back button after going from root page to some topic page is not taking me back to root page

issue72problem1.mov

Solution - In HomePage , watch params. If topic changes and is undefined then assign it the rootSlug value

Issue 2 - Clicking App Back Icon is pushing a new page with parent slug

issue72problem2.mov

Solution - It should be consistent with browser back button endless ui

After fixing the issue

issue72solution.mov

Update[4th commit]- Fixed Inconsistent Scroll Behaviour
After trying a lot of solutions,
for now this seems to be the best one without a big change in router configuration or project structure.

fixedInconsistentScrollBehaviour.mov

Copy link
Collaborator

@benoit74 benoit74 left a comment

Choose a reason for hiding this comment

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

Very good point.

Please fix issues mentioned by the CI (errors and warnings) before I can merge.

@benoit74 benoit74 changed the title Partly solves Issue #72 Harmonize and partly fix back buttons behavior Mar 4, 2024
@githyuvi
Copy link
Contributor Author

githyuvi commented Mar 4, 2024

@benoit74 Line 68 defines the parentSlug() method. It is not being used now hence giving CI errors. Should I remove it or keep it as it is a utility.

@benoit74
Copy link
Collaborator

benoit74 commented Mar 4, 2024

You can (should) remove everything which is not used anymore

@githyuvi githyuvi requested a review from benoit74 March 4, 2024 14:30
@githyuvi
Copy link
Contributor Author

githyuvi commented Mar 4, 2024

Okay 👍

Copy link
Collaborator

@benoit74 benoit74 left a comment

Choose a reason for hiding this comment

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

Why do you need the new dataLoaded property? You did not spoke about this new change, and we already have a vf-if on topic: <div v-if="topic" class="content">? (I probably miss something)

@githyuvi
Copy link
Contributor Author

githyuvi commented Mar 4, 2024

const fetchData = async function () {
dataLoaded.value = false
if (props.slug == undefined) {
return
}
const resp = await main.fetchTopic(props.slug)
if (resp) {
topic.value = resp
}
dataLoaded.value = true
}

This code runs after every change in url parameter.
Page content changes only after topic value changes again. Due to which we are getting the behaviour shown in the below video.
Page scrolls instantly to top and we can see topic name changing from English to African Storybook...
This video demonstrates the problem

Screen.Recording.2024-03-04.at.8.54.21.PM.mov

If we use v-if = "dataLoaded" on topic title div and TopicSection component then we can hide current page content when we are waiting for topic value to change which I suppose should be the ideal behaviour.
Please give your suggestions on this.

@benoit74
Copy link
Collaborator

benoit74 commented Mar 4, 2024

This code runs after every change in url parameter.
Page content changes only after topic value changes again. Due to which we are getting the behaviour shown in the below video.
Page scrolls instantly to top and we can see topic name changing from English to African Storybook...
This video demonstrates the problem

Perfect, then your fix is appropriate.

Could you fix CI (again, sorry)? Do you have activate pre-commit with pre-commit install on your machine ? This should catch most QA issues, both Python and TS

@githyuvi
Copy link
Contributor Author

githyuvi commented Mar 4, 2024

No, thanks for the info. I will install

@githyuvi
Copy link
Contributor Author

githyuvi commented Mar 8, 2024

@benoit74 I have made the requested changes. Please check

@benoit74 benoit74 self-requested a review March 8, 2024 09:03
Copy link
Collaborator

@benoit74 benoit74 left a comment

Choose a reason for hiding this comment

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

Still one CI issue around the scrollTo property of Vue.Router.

You should probably import the ScrollPositionCoordinates from vue-router and use this to indicate the proper type. Something like { top: 0, behavior: 'instant' } as ScrollPositionCoordinates would probably do the trick (just run yarn build locally to confirm, it should probably reproduce the issue).

See https://github.com/vuejs/router/blob/8618943e3c4e671da1ff3a32764a910db5835331/packages/router/src/scrollBehavior.ts#L45 and https://github.com/vuejs/router/blob/8618943e3c4e671da1ff3a32764a910db5835331/packages/router/src/scrollBehavior.ts#L11

@githyuvi
Copy link
Contributor Author

githyuvi commented Mar 8, 2024

@benoit74 I went through it. For some reason, behaviour accepts both smooth and instant values but accepting only smooth during build. I will check it again.

@benoit74
Copy link
Collaborator

benoit74 commented Mar 8, 2024

Good luck, this might be a nasty Typescript issue ...

@githyuvi
Copy link
Contributor Author

githyuvi commented Mar 8, 2024

I went through this issue and it seems typescript has removed support for behaviour: "instant".
If we use behaviour: "smooth" there is a bottom to top scroll animation when going back to previous page.

Screen.Recording.2024-03-08.at.4.54.25.PM.mov

@benoit74 What do you suggest?

@benoit74
Copy link
Collaborator

Looking at the issue you found (good work!) it looks like instant has been renamed to auto. Any rationale to prefer smooth instead of auto? At this stage, I really do not mind, more an open question to document the reasoning here, should we have to come back on this decision it might help. That been said, I do not mind TbH, so just update the PR with what is best according to your understanding.

@githyuvi
Copy link
Contributor Author

@benoit74 scroll-behavior default value is auto but bootstrap is overriding it to smooth.
bootstrapSettingScrollBehaviour

If we change that globally in style.css. then we can use auto wherever we can instant scroll and smooth for smooth. But same is not possible if root level scroll-behaviour is smooth.

@githyuvi githyuvi changed the title Harmonize and partly fix back buttons behavior Harmonize and fix back buttons behavior and inconsistent scroll behaviour Mar 11, 2024
@benoit74
Copy link
Collaborator

OK, then let's go for smooth for now. Please update the PR.

@githyuvi
Copy link
Contributor Author

changed it to smooth and updated the pr

@benoit74 benoit74 self-requested a review March 12, 2024 13:25
Copy link
Collaborator

@benoit74 benoit74 left a comment

Choose a reason for hiding this comment

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

LGTM

@benoit74 benoit74 merged commit 31691b1 into openzim:new_ui Mar 12, 2024
6 checks passed
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.

2 participants