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

Add a min-height to stop the back to top link collapsing #787

Closed
wants to merge 1 commit into from

Conversation

steventux
Copy link
Contributor

@steventux steventux commented Feb 21, 2018

Addresses #786

A good test page is https://government-frontend-pr-787.herokuapp.com/drug-device-alerts/drug-alert-amoxicillin-sodium-500mg-powder-for-solution-for-injection-additional-reports-of-injection-site-reactions-in-paediatrics-and-adults?cachebust=1519124601

Here's a video of it misbehaving:

contents-moving

The elements below the back to top link can briefly
appear above the link as it transitions from an in-body
element to a fixed position, adding a minimum height stops the
element collapsing and causing this flickering element
reordering.

Thanks to @andysellick and @fofr for reporting and explaining this.


Component guide for this PR:
https://government-frontend-pr-787.herokuapp.com/component-guide

The elements below the back to top link can briefly
appear above the link as it transitions from an in-body
element to a fixed position, adding a minimum height stops the
element collapsing and causing this flickering element
reordering.
@fofr
Copy link
Contributor

fofr commented Feb 23, 2018

Looks like this isn't currently working as expected.

@steventux
Copy link
Contributor Author

@fofr can you elaborate? I've tested this in several browsers now, the original commit wasn't successful as it was a bit simplistic but since 64b5e94 I can't replicate the same jumpy behaviour. I've now tested on Safari 8, Chrome 64 and IE9. I've been able to replicate it (intermittently) on Firefox 58 if I use arrow keys but otherwise scrolling no longer produces a noticeable jump. Perhaps I'm used to working around the issue? Any info on how to replicate would be handy.

Would you say this latest commit is an improvement at all? If so I'd be inclined to accept this short term fix until we have time to dive a bit deeper into the problem.

@vanitabarrett
Copy link
Contributor

@steventux @fofr Possibly a caching issue - I thought I could replicate on https://government-frontend-pr-787.herokuapp.com/drug-device-alerts/drug-alert-amoxicillin-sodium-500mg-powder-for-solution-for-injection-additional-reports-of-injection-site-reactions-in-paediatrics-and-adults?cachebust=1519124601 on Chrome too, but then refreshed and the problem seems to be resolved with the latest commit.

@steventux
Copy link
Contributor Author

@vanitabarrett would you say this improves the behaviour? I think if we've improved the behaviour this is a harmless if slightly simplistic way of improving user experience.

@vanitabarrett
Copy link
Contributor

@steventux It definitely seems to improve the problem - I've just quickly checked on latest versions of Safari, Firefox and Chrome, and IE 9 on browserstack, and I can't replicate the issue any more

@steventux
Copy link
Contributor Author

@fofr @vanitabarrett this PR could do with approval if it's deemed to at least improve the behaviour.

@vanitabarrett
Copy link
Contributor

@steventux I can't seem to replicate this issue any more on the latest master build of government-frontend so I'm wondering if this fix is still needed? Testing in Chrome 64

Copy link
Contributor

@maxgds maxgds left a comment

Choose a reason for hiding this comment

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

I can't replicate the behaviour using this branch, I think we should merge as it's an improvement. If we find further issues we can address them separately.

@steventux
Copy link
Contributor Author

Very strange, I know some small stylistic tweaks were applied to the feedback component, perhaps the additional margin before the feedback component/after the content has negated the space where the two elements appeared to switch place. I'll close this then, thanks 👍

@steventux steventux closed this Mar 1, 2018
@barrucadu barrucadu deleted the fix-back-to-top-link-jumpy-jumpy branch April 27, 2018 12:46
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.

5 participants