Skip to content
This repository has been archived by the owner on May 19, 2020. It is now read-only.

[ui] Add infinite pagination/scrolling in subnet's visual hosts view #66 #89

Merged
merged 1 commit into from
Feb 23, 2020

Conversation

pawelplsi
Copy link
Contributor

Should fix #66

@pawelplsi pawelplsi force-pushed the master branch 7 times, most recently from 5f1b980 to fbce0b5 Compare January 19, 2020 02:10
@coveralls
Copy link

coveralls commented Jan 19, 2020

Pull Request Test Coverage Report for Build 351

  • 98 of 98 (100.0%) changed or added relevant lines in 5 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 345: 0.0%
Covered Lines: 567
Relevant Lines: 567

💛 - Coveralls

@pawelplsi
Copy link
Contributor Author

pawelplsi commented Jan 19, 2020

I have implemented this feature the way wanted there: #66 (comment), there are only 5 pages in DOM tree at time to reduce memory usage, however a few measurements have shown me that keeping these pages as a JS object doesn't really consume much memory compared to the entire page, so in this implementation they are stored this way client-side so there's no need to fetch one page from server multiple times.
The only drawback of this implementation I've found is that when we remove pages from the beginning (to save memory), layout of lists items representing hosts may be shifted a bit (depending on window width), and since browser size may change, I don't have any idea to prevent such behavior, I've already tried to replace these list items with placeholders in order to keep the layout as it is, but it turned out not to be memory efficient enough.

I see visualization is disabled on IPV6 networks, should I make it work?

@pawelplsi pawelplsi marked this pull request as ready for review January 19, 2020 02:37
@atb00ker
Copy link
Member

I created a really large subnet like 11.0.0.0/7 and the page is auto loading all the ?page=X, even without scrolling, does it happen on your end as well? 😄

@pawelplsi
Copy link
Contributor Author

pawelplsi commented Jan 19, 2020

@atb00ker No, it's woking with such subnet on my end, could you tell what browser do you use and how does it exactly behave? I have tested in both on firefox and chromium.

@pawelplsi pawelplsi force-pushed the master branch 2 times, most recently from 19a7cb8 to 61d3cde Compare January 19, 2020 13:09
@atb00ker
Copy link
Member

Hi, sorry for sharing a google drive link but I don't have the time to debug this so I am attaching a video,
I am using Chrome:79.0.3945.88
Link : https://drive.google.com/file/d/1dZNxzvLDDGVda7G5ARHCSG5c2Xf-A20c/view?usp=sharing

I tested on Firefox 68.3.0esr, it worked fine.

@atb00ker
Copy link
Member

The behaviour isn't smooth, it's hard to navigate from one subnet to another or to find a specific subnet.

@nemesisdesign what do you think?

I am thinking to peek into the implementation of phpipam and see their solution instead.

@pawelplsi
Copy link
Contributor Author

I changed that implementation a bit, now start of every page is aligned to the left, it may look a bit worse, but with this approach it's no longer 'hopping' when scrolling. I also turned off browser's automatic scroll anchoring and replaced it with a code that manually prevents random scrolling, It should fix your issue on chrome @atb00ker, could you check if it did?

@pawelplsi pawelplsi force-pushed the master branch 2 times, most recently from 7d6d630 to 693a76f Compare January 20, 2020 19:03
Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

I see good progress!

Please make sure this feature does not impact subnets which have a manegeable size, for example, standard /24 subnets should look very similar to how they looked before.

For some reason I can't scroll by using my mouse, does it happen to you as well?

Copy link
Member

@atb00ker atb00ker left a comment

Choose a reason for hiding this comment

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

@pawelplsi @nemesisdesign

I tested it, it works! 😄

For some reason I can't scroll by using my mouse, does it happen to you as well?

When using a scrollbar inside a page, the behaviour is really flaky and dependant on your type of pointer & browser, that's why scroll highjacking is discouraged at some places.

I have 2 mouses,

  1. Internal one scrolls on focus, if curser is inside the subnet section, it scrolls that section.
  2. External mouse requires me to hold right button while scrolling, try doing that, it might work for you! 😄

@pawelplsi
Copy link
Contributor Author

@nemesisdesign
Scrolling by mouse works, but you need to focus the scroll container first (at least on FF), and yes, it is a bit quirky but we cannot make it perfect when scrolling with mouse since new pages are added so the scroll bar is not fixed-size.
For small subnets it looks as it used to, the only difference is that there is a scroll container, i can make it bigger, the scrollbar would not appear then in small subnets

@nemesifier
Copy link
Member

@pawelplsi I mean I cannot scroll by pressing the mouse left button and dragging it up or down

@pawelplsi
Copy link
Contributor Author

I mean I cannot scroll by pressing the mouse left button and dragging it up or down

Huh, the scrollbar was partially covered with right menu/drawer object, one of most annoying things I debugged recently 😄

@pawelplsi pawelplsi force-pushed the master branch 3 times, most recently from 7c9285c to 227ced1 Compare January 21, 2020 00:00
Copy link
Member

@atb00ker atb00ker left a comment

Choose a reason for hiding this comment

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

LGTM! 😄
Final testing done on chrome and firefox! 😄

Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

Wait a minute, why is IPv6 not implemented?
I think in the past we excluded ipv6 subnets exactly because they would not load since the page would be too big.
Since we solved that problem with this solution we should support IPv6 as well!

@nemesifier nemesifier added the enhancement New feature or request label Jan 24, 2020
@nemesifier nemesifier added this to To do (general) in OpenWISP Contributor's Board via automation Jan 24, 2020
@pawelplsi pawelplsi force-pushed the master branch 4 times, most recently from 5a1b2b9 to a8556df Compare January 26, 2020 23:10
@pawelplsi
Copy link
Contributor Author

I added IPv6 support, that let me simplify some logic also, and wrote a custom pagination (start adddress parameter instead of page number) that will be more convenient later in fixing #91, now I only need to write some more tests to increase the coverage.
But I'm not sure about one thing, should that pagination and host set implementation be in generics.py? Do you think I should move it somewhere?

@atb00ker
Copy link
Member

I just noticed that the spacing is not even in the last line of each page, it's not a blocking issue, but if it's easily fixable, please look into it...
send

…penwisp#66

Added infinite scrolling, added visualization in IPv6 subnet edit form, optimized subnet chart data queries.
Should fix openwisp#66
@pawelplsi
Copy link
Contributor Author

@atb00ker added fixed width margins to these cells, it seems to be fixed now 😄

Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

Great work!

I am going to apply a small css fix (a28b66c) to resolve an issue that causes a line break at each page. While I applied that fix in my dev branch I noticed another issue for which I opened an issue for:
#96.

Another follow up is to add support for this into openwisp-ipam: openwisp/openwisp-ipam#24.

@pawelplsi I think you're the best person right now to fix it, it's not a big deal, it's just a matter of changing the HTML tags. The rest of the functionality should be intact.

@nemesifier nemesifier merged commit 3405008 into openwisp:master Feb 23, 2020
OpenWISP Contributor's Board automation moved this from To do (general) to Done Feb 23, 2020
@pawelplsi
Copy link
Contributor Author

Thanks for your appreciation @nemesisdesign, but are you sure inlining page divs is the right way to go? I have not done it that way by an accident. The pages are not always as wide as your screen is, so after a page is removed from the beginning (to save memory) the pages are aligned differently, so the hosts are 'hopping' through the list, as I explained in comments above, I tried to solve it but I didn't manage to do it any efficient way, and I think it's possible there's no 'not-dirty' way to solve that. Although the line breaks may not look really well to you, I think that the problem above is a way more annoying.

@nemesifier
Copy link
Member

@pawelplsi occasional hops while scrolling an infinite scroller is pretty common, but if you can find a solution that solves both problems (line break and hopping) would be great!

Otherwise we can live with the occasional hopping, the line-break is too obvious to keep, it makes it look unprofessional.

pawelplsi added a commit to pawelplsi/django-ipam that referenced this pull request Feb 25, 2020
Fixes little issue with hosts list href/align/styling
Fixes openwisp#89
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

[bug] Subnet page for /8 or /16 are not accessible
4 participants