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

Implemented pagination on app details page #119

Merged
merged 15 commits into from
Nov 11, 2020

Conversation

LilyLME
Copy link
Contributor

@LilyLME LilyLME commented Oct 16, 2020

As required in #88 pagination has been added to reduce scroll on very long pages.

Tasks

  • Story book
  • Use TailwindCSS

Copy link
Contributor

@wlach wlach left a comment

Choose a reason for hiding this comment

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

This looks like a good start. Just some initial thoughts.

src/pages/AppDetail.svelte Outdated Show resolved Hide resolved
src/pages/AppDetail.svelte Outdated Show resolved Hide resolved
@LilyLME
Copy link
Contributor Author

LilyLME commented Oct 18, 2020

Pagination view

Screenshot from 2020-10-18 19-00-13

Copy link
Contributor

@wlach wlach left a comment

Choose a reason for hiding this comment

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

Thanks for your persistence on this! It turns out this is a rather complex issue to solve. I've put in some more feedback, hopefully it is helpful.

src/pages/AppDetail.svelte Outdated Show resolved Hide resolved
src/components/Pagination.svelte Show resolved Hide resolved
src/components/Pagination.svelte Outdated Show resolved Hide resolved
src/components/Pagination.svelte Outdated Show resolved Hide resolved
src/components/Pagination.svelte Outdated Show resolved Hide resolved
src/pages/AppDetail.svelte Outdated Show resolved Hide resolved
src/components/Pagination.svelte Show resolved Hide resolved
Signed-off-by: LilyLME <[email protected]>
Signed-off-by: LilyLME <[email protected]>
Signed-off-by: LilyLME <[email protected]>
Signed-off-by: LilyLME <[email protected]>
@LilyLME LilyLME requested a review from wlach November 2, 2020 21:32
Copy link
Contributor

@wlach wlach left a comment

Choose a reason for hiding this comment

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

Thanks for your persistence on this, and sorry for the slowness of my response. I think this is actually pretty close, after taking a second look at this, I think you may have been right about some aspects (e.g. the need to keep more state in the module calling the pagination component). To keep things more managable, I think we can keep track of the various variables inside a paginationState variable. Could you give that a try?

src/pages/AppDetail.svelte Outdated Show resolved Hide resolved
src/pages/AppDetail.svelte Outdated Show resolved Hide resolved
src/pages/AppDetail.svelte Outdated Show resolved Hide resolved
src/pages/AppDetail.svelte Outdated Show resolved Hide resolved
src/components/Pagination.svelte Show resolved Hide resolved
@LilyLME
Copy link
Contributor Author

LilyLME commented Nov 4, 2020

Hello @wlach, I have updated this as per requirements discussed. The calling module maintains it's level of state but now very neatly embedded in a single state object.

I would have loved if the components itself could let objects with properties be exported, but looks like it's a difficult problem trying to figure out how attributes within objects change :
See :

@LilyLME LilyLME requested a review from wlach November 4, 2020 00:09
@wlach wlach marked this pull request as ready for review November 4, 2020 14:57
Copy link
Contributor

@wlach wlach left a comment

Choose a reason for hiding this comment

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

After testing this out a bit, I'm noticing a few problems:

  1. There's an unpaginated list of metrics on top of the paginated one (might just be a merging artifact), this should be easy to fix.
  2. The pagination behaviour doesn't play nicely with the filtering. Basically we want to paginate through any list of items that the user has filtered (you can test with a string like "search" with the Fenix browser: http://localhost:5000/#!/apps/fenix)

Thanks again for your hard work on this.

src/components/Pagination.svelte Outdated Show resolved Hide resolved
src/components/Pagination.svelte Outdated Show resolved Hide resolved
@LilyLME
Copy link
Contributor Author

LilyLME commented Nov 4, 2020

@wlach all last change requests made. The pagination now plays nicely with the filteredMetrics also.

@LilyLME LilyLME requested a review from wlach November 4, 2020 19:01
Copy link
Contributor

@wlach wlach left a comment

Choose a reason for hiding this comment

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

This is getting closer for sure, but I found a bug:

Neither the search or the skip will work and I see this in the console:

image

@LilyLME
Copy link
Contributor Author

LilyLME commented Nov 11, 2020

Hello @wlach sorry for taking a while with this, can you review again and check for the last the bug so we move on?

@LilyLME LilyLME requested a review from wlach November 11, 2020 10:51
Copy link
Contributor

@wlach wlach left a comment

Choose a reason for hiding this comment

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

Thanks, this works great now!

@wlach wlach merged commit 6edcdd4 into mozilla:main Nov 11, 2020
@wlach
Copy link
Contributor

wlach commented Nov 11, 2020

Some quick followup tasks for this:

@LilyLME feel free to work on these (but obviously feel no obligation). I'll make a note to file some issues on these in the future (today is technically a holiday for me)

@LilyLME
Copy link
Contributor Author

LilyLME commented Nov 12, 2020

@wlach that's fine, Since the simpler and faster thing to do would be adding the pagination to ping page, I would rather start with that before looking a lodash' chunk.

  • You also had a storybook requirement for the pagination component is that pressing?

@LilyLME LilyLME deleted the pagination-app-details branch November 12, 2020 08:04
@wlach
Copy link
Contributor

wlach commented Nov 12, 2020

@wlach that's fine, Since the simpler and faster thing to do would be adding the pagination to ping page, I would rather start with that before looking a lodash' chunk.

For sure. For what it's worth, I think the lodash chunk task should be quite easy. Mostly just a matter of running npm install lodash and then using it.

* You also had a storybook requirement for the pagination component is that pressing?

Ah yes! We definitely want that too.

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