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 admin UI anchor link scroll bug #99

Merged
merged 4 commits into from
Jun 1, 2018
Merged

Conversation

benkmugo
Copy link
Member

  • fixed height on the width controller element causes the user to get
    'stuck' and unable to scroll back to the top when clicking on a link
    targeting an in page anchor

The bug is also noted in this Jira ticket:
https://jira.ez.no/browse/EZP-29210

- fixed height on the width controller element causes the user to get
  'stuck' and unable to scroll back to the top when clicking on a link
targeting an in page anchor
@pkamps
Copy link
Member

pkamps commented May 30, 2018

There is a side effect. You can drag the left column separator in order to adjust the left column width. With this patch only the first 400px height of the separator is draggable with the mouse. But everything after the 400px height is not draggable anymore with the mouse.
It's a bit tough to describe, I can do a quick demo if necessary.

Copy link
Member

@pkamps pkamps left a comment

Choose a reason for hiding this comment

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

I think it needs to get fixed in a different way. See my comment about the side effect for this patch.

@benkmugo
Copy link
Member Author

I see what you mean @pkamps. The part of the gray vertical bar that can be dragged depends on the height of the content structure menu now, since the height is set to auto.

I didn't realize that was an issue as it was tested on a site with a lot of content.

Once a workaround is in place I'll update. Thanks.

- moved widthcontrol-handler element out of menu tpl and into pagelayout
  left-separator
- updated widthcontrol-handler element styles for new markup position
- removed widthcontrol-handler elements from all other admin design menu
  templates
@benkmugo benkmugo removed the request for review from peterkeung May 31, 2018 01:24
@benkmugo
Copy link
Member Author

@pkamps the new commit is more involved than the initial fix attempt.
Due to the position in the markup and the parent container styles I wasn't able to get the width handler to stretch to 100% where it was in the markup.

Moving it into the left separator accomplishes the full height consistently and does not appear to be interfering with the functionality. That is the event handlers pick up the element in its new position and behave as expected.

I have removed the width control element from the other menu includes in the admin design as they're no longer needed. Even with those elements still in place the update worked ok as the additional handler elements simply remained hidden.

Could you please check to see if:

  • the styles work ok (no visual artifacts or mis-alignment)
  • the drag behaviour works ok (drag-able on border and tab element)
  • the drag behaviour works across all admin sections where it did before the patch (e.g. content structure, user accounts, media, setup ...)

If you have any questions or concerns, please let me know. Thanks.

@pkamps
Copy link
Member

pkamps commented May 31, 2018

It is so much more elegant to have the draggable handler directly in the separator div. Great solution. I like how it removes the redundant code as well.
Small thing - feel free to ignore it: There is no need to explicitly set the hight to "auto", is there? I think you can remove that style.

@benkmugo
Copy link
Member Author

I'll add another commit to remove the redundant height rule and then we can merge it in.

- removed obsolete height style from widthcontrol
- removed unit suffix from widthcontrol top position
@benkmugo
Copy link
Member Author

benkmugo commented Jun 1, 2018

@pkamps third time lucky. Should be good to go now.

@pkamps pkamps merged commit 03ba227 into master Jun 1, 2018
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