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

Votelog List Pages #84

Merged
merged 5 commits into from
Nov 8, 2019
Merged

Votelog List Pages #84

merged 5 commits into from
Nov 8, 2019

Conversation

mixth
Copy link
Contributor

@mixth mixth commented Nov 8, 2019

For #74. I create Votelog List pages at \votelog and \votelog\page\${i} and also pagination on the bottom.

I set number of votelogs per page at 4, to show the pagination. We will have to update it to 8 at launch.

Screenshot

Screenshot_2019-11-08 บันทึกการประชุมและการลงมติ

Future Work

  • The pagination will increase and get ugly over time.
  • Current styling for VoteLogCard is not compatible for smaller screens.
  • The info on the top, including update date, is not populated.

Concern

  • With \votelog\page\${i}, there is a slim but possible chance of collision with Votelog's slug.

As always, please let me know if this needs any changes :)

@rapee
Copy link
Contributor

rapee commented Nov 8, 2019

Thanks for this PR. @mixth

One comment, I think pagination should be dynamic e.g. no scrolling to top and no page reload. Currently it's more like we have separate pages bound together.

It works for now so I'll merge it.

The pagination will increase and get ugly over time.

We'll handle this case together with pagination on other pages once and for all in the future. #86

Current styling for VoteLogCard is not compatible for smaller screens.

This will spawn a new issue to handle mobile view for VoteLogCard #87

The info on the top, including update date, is not populated.

Mock is good. We'll have another sprint for update date. #88

With \votelog\page${i}, there is a slim but possible chance of collision with Votelog's slug.

As mentioned, when we do dynamic pagination for this, we'll need only /votelog?page=1 so no URL conflicts. What do you think? #86

@rapee rapee merged commit 9b7264c into electinth:master Nov 8, 2019
@rapee rapee mentioned this pull request Nov 8, 2019
@mixth
Copy link
Contributor Author

mixth commented Nov 8, 2019

@rapee I was thinking about dynamically switching page too. But I thought we would prefer it to be more "static". 😅

As mentioned, when we do dynamic pagination for this, we'll need only /votelog?page=1 so no URL conflicts. What do you think?

LGTM

@mixth mixth deleted the votelog-list-page branch November 12, 2019 17:22
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