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

[RTM] Replaced the BE_PAGE_OFFSET cookie with sessionStorage #467

Merged
merged 13 commits into from May 8, 2019

Conversation

Projects
None yet
5 participants
@Toflar
Copy link
Member

commented May 1, 2019

No description provided.

@leofeyer

This comment has been minimized.

Copy link
Member

commented May 2, 2019

Great work with some clever solutions! 🦊

I wonder if we should use session storage instead of local storage here though, because a) the scroll offset is short-term information and b) the scroll offset should not be shared between tabs. WDYT?

@Toflar

This comment has been minimized.

Copy link
Member Author

commented May 2, 2019

Yeah I was thinking about that too but I didn't know 😄 Until now, as it was a cookie, the information was shared but I agree it makes no sense because you would probably scroll to somewhere in nowhere when you work with two tabs. I'll switch to sessionStorage 👍

@Toflar

This comment has been minimized.

Copy link
Member Author

commented May 2, 2019

Done, it's sessionStorage now. And I've also reworked the additional scroll pixels again to automatically calculate them instead of a hardcoded height. Currently it scrolls way to much down on mobile where the pastehint is not displayed (but still present, so it currently adds 40 pixels).

So apart from removing the cookie the solution is now a lot better because

  • it calculates the height of the additional elements automatically and only when needed
  • it does not share the scroll information across tabs

😎

@Toflar Toflar changed the title Replaced the BE_PAGE_OFFSET cookie with localstorage [RTM] Replaced the BE_PAGE_OFFSET cookie with localstorage May 2, 2019

@Toflar Toflar changed the title [RTM] Replaced the BE_PAGE_OFFSET cookie with localstorage [RTM] Replaced the BE_PAGE_OFFSET cookie with sessionStorage May 2, 2019

Show resolved Hide resolved core-bundle/src/Resources/public/core.js Outdated
Show resolved Hide resolved core-bundle/src/Resources/contao/drivers/DC_Folder.php Outdated
@leofeyer
Copy link
Member

left a comment

There are still two bugs. The script works perfectly on the "edit" page but unfortunately not on the listing page. Watch this video before reading on:

https://www.dropbox.com/s/7tjw5orx0c886wv/scroll-offset.mov?dl=0

  1. After I have clicked the "cut" icon, the URL changes, so I had to remove the window.location != chunks[2] check to make it work at all.

  2. As you can see, the paste hint adds too much scroll offset. The element height is calculated as 60 pixels, however, the real offset is only 22 pixels.

To fix 2), we could change the data attribute to data-add-scroll-offset="22", couldn't we?

@Toflar

This comment has been minimized.

Copy link
Member Author

commented May 3, 2019

1. After I have clicked the "cut" icon, the URL changes, so I had to remove the `window.location != chunks[2]` check to make it work at all.

But if we do that, it would not get reset for e.g. saveNback, right?

As you can see, the paste hint adds too much scroll offset. The element height is calculated as 60 pixels, however, the real offset is only 22 pixels.
To fix 2), we could change the data attribute to data-add-scroll-offset="22", couldn't we?

Well, I've removed the hardcoded pixel height of 40 because I wanted it to be smarter than now. Right now it scrolls to the wrong position too then because it just always scrolls 40 pixels. I used the scroll height, not the element height? Maybe there's a better option than el.getScrollSize().y? I've tried with el.getDimensions() and el.getComputedSize() etc. but they all returned 0 because the paste hint div itself has no height, the children do. I'm pretty sure there must be some JS element property that contains the height of all children maybe? @ausi ?

@leofeyer

This comment has been minimized.

Copy link
Member

commented May 3, 2019

  1. After I have clicked the "cut" icon, the URL changes, so I had to remove the window.location != chunks[2] check to make it work at all.

But if we do that, it would not get reset for e.g. saveNback, right?

I have only removed the code for the video. The URL check has to stay, but it has to be fixed for the listing page.

To fix 2), we could change the data attribute to data-add-scroll-offset="22", couldn't we?

Well, I've removed the hardcoded pixel height of 40 because I wanted it to be smarter than now. […] I'm pretty sure there must be some JS element property that contains the height of all children maybe? @ausi ?

But that would not fix my issue. I need the additional scroll offset to be less than the element height.

@Toflar

This comment has been minimized.

Copy link
Member Author

commented May 3, 2019

But that would not fix my issue. I need the additional scroll offset to be less than the element height.

Hardcoding it to 22 would also be wrong though because in theory (not for the paste hint but maybe for any other element) when I shrink the screen size the element gets higher, right? So it would be wrong again. Can you tell why you want to add less pixels than the height of the element?

@leofeyer

This comment has been minimized.

Copy link
Member

commented May 3, 2019

We do not need to hardcode it. The attribute value can be optional; if it is set, we use it, and if not, we measure the element's height.

Can you tell why you want to add less pixels than the height of the element?

The paste hint is positioned relatively and uses less space than calculated by el.getScrollSize().y. As I said, the table only moves down by 22 pixels, even though el.getScrollSize().y returns 60.

@asaage

This comment has been minimized.

Copy link

commented May 3, 2019

60px is correct for the height of #paste_hint's children (font-size+padding+margin+rotation)
but as the position is absolut it's height doesn't matter at all.
the shift is caused by the listing_container itself.

#paste_hint+.tl_listing_container {
    margin-top: 30px; */
}
#tl_buttons+.tl_listing_container {
    margin-top: 10px;
}
@Toflar

This comment has been minimized.

Copy link
Member Author

commented May 3, 2019

Okay I've reworked this here. We're now storing the offset only and I've added events on the buttons to delete the entry (except for save). This now works for me. Is the scroll offset still an issue for you? For me it calculates 44px and I'm exactly at the correct position then.

@leofeyer

This comment has been minimized.

Copy link
Member

commented May 6, 2019

Is the scroll offset still an issue for you?

Unfortunately yes. 🙈

I have probably not explained well what I consider to be the problem, so here are some screenshots.

The yellow line is where the cursor is. Now when I click the "cut" icon, I want the page to be at the exact same vertical position (current behavior):

With your changes, however, the page gets scrolled down too much:

IMHO, this can only be fixed with data-add-scroll-offset="…", because as @asaage explained, the offset is not caused by the paste hint but by moving the main view down via CSS:

#paste_hint+.tl_listing_container {
    margin-top: 30px;
}
@asaage

This comment has been minimized.

Copy link

commented May 6, 2019

In Articles and Pages (preceeding a .tree_view) the #paste_hint looks a little off anyway (compared to ContentElements).
That's because of .tl_listing_container.tree_view { position: relative; }

image
image

Maybe it's easier to get rid of the absolut-positioning and the differing margins?

#paste_hint+.tl_listing_container {
    /* margin-top: 30px; */
}
#paste_hint {
    /* position: relative; */
}
#paste_hint p {
    /* position: absolute; */
    /* top: 0; */
    /* right: 30px; */
    transform-origin: top right;
    text-align: right;
    margin-right: 30px;  /*?*/
    margin-bottom: -26px; /*?*/
}
/* untested*/
Show resolved Hide resolved core-bundle/src/Resources/public/core.js Outdated
initScrollOffset: function() {
// Kill the legacy cookie here; this way it can be sent by the server
// but it wont be resent by the client in the next request
Cookie.dispose('BE_PAGE_OFFSET');

This comment has been minimized.

Copy link
@aschempp

aschempp May 6, 2019

Contributor

Are you sure this works and the cookie is never "http-only"?

This comment has been minimized.

Copy link
@Toflar

Toflar May 7, 2019

Author Member

It won't work if someone sets the cookie http-only. I think the use case is just the one where people copied the code to their own DC_* classes or so. I guess if they did, they just copied System::setCookie('BE_PAGE_OFFSET', 0, 0);. We could also just adjust System::setCookie() and make it ignore the BE_PAGE_OFFSET cookie, wdyt @leofeyer?

This comment has been minimized.

Copy link
@Toflar

Toflar May 7, 2019

Author Member

Well of course it was not http-only so far. Backend.getScrollOffset() set the cookie so it was never http-only. It could've only been http-conly if you set it using System::setCooke() which - if you did so - would've lead to weird behaviour. So I guess this is good then.

Show resolved Hide resolved core-bundle/src/Resources/public/core.js Outdated
@Toflar

This comment has been minimized.

Copy link
Member Author

commented May 7, 2019

@leofeyer there you go. You can now use

  • data-add-scroll-offset - will just add the current scroll size to the offset.
  • data-add-scroll-offset="20" - will add 20 pixels to the offset.
  • data-add-scroll-offset="-20" - will subtract 20 pixels from the offset.
  • data-add-scroll-offset="20%" - will add 20 percent of the current scroll size to the offset.
  • data-add-scroll-offset="-20%" - will subtract 20 percent of the current scroll size from the offset.

So you should be able to configure whatever you need now 😄

@leofeyer

This comment has been minimized.

Copy link
Member

commented May 8, 2019

If you agree with my changes in 8b280fc, I would rebase the branch and merge it now.

@Toflar

This comment has been minimized.

Copy link
Member Author

commented May 8, 2019

I do but you did not update the definitions in DC_Table so it should still scroll down too far?

@leofeyer

This comment has been minimized.

Copy link
Member

commented May 8, 2019

Ah, you are right. 👍 Added in 22cae99.

@leofeyer leofeyer force-pushed the Toflar:feature/remove-be-page-offset-cookie branch from 22cae99 to 66b106b May 8, 2019

@Toflar

This comment has been minimized.

Copy link
Member Author

commented May 8, 2019

Excellent, this is RTM then 👍
👋 to yet another pointless cookie 😎

@leofeyer leofeyer merged commit 67ca3ba into contao:master May 8, 2019

3 checks passed

Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
coverage/coveralls Coverage increased (+0.009%) to 88.733%
Details
@leofeyer

This comment has been minimized.

Copy link
Member

commented May 8, 2019

Very nice! Thank you @Toflar.

@Toflar Toflar deleted the Toflar:feature/remove-be-page-offset-cookie branch May 8, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.