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

Add page selection menu #5523

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Add page selection menu #5523

wants to merge 3 commits into from

Conversation

ongsalt
Copy link

@ongsalt ongsalt commented May 9, 2023

related #5518
On mobile when click at the page number the page selection menu will show up. On desktop this menu can be trigger using ... button in pagination instead.

I also add Utility.isMobile to determine user platform based on screen size similar to css .mobile-only

Check if screen width is less than 660px or not
inspire by css class .mobile-only
@BrokenEagle
Copy link
Collaborator

Yo man... you're going to want to squash this down to a single commit to have any hope of this ever being merged.

@ongsalt
Copy link
Author

ongsalt commented May 11, 2023

Yo man... you're going to want to squash this down to a single commit to have any hope of this ever being merged.

Done. I'm kinda new to this may I ask why. Also I don't know if i should include b41563b or not

@BrokenEagle
Copy link
Collaborator

Yeah, don't include b41563b. If any merging needs to be done, evazion can do it on his side. However, if there are any merge conflicts, then you'll have to resolve them yourself and update the commit.

As for why this is done, it's cleaner and easier to follow. Basically, each commit should encapsulate one entire change to the system, whether it's a minor fix or a feature add. If a pull request has multiple new features and/or changes, then it's fine to break them up each of them up into their own separate commits.

- also make current page number have click feedback.
@ongsalt
Copy link
Author

ongsalt commented May 12, 2023

Is this okay?

@BrokenEagle
Copy link
Collaborator

For the Utility.isMobile function, use the window.screen variable instead, so as not to falsely identify browers on desktop computers which have had their widths shrunk beneath the 660px cutoff.

Utility.isMobile = function () {
  window.screen.width <= 660 || window.screen.height <= 660;
}

I went and added a check for the height as well, since many of your newer devices have a screen height greater than 660, in which case it would falsely detect it as being a desktop when the user has the phone rotated horizontally.

It's outside the scope of this PR, but the mobile CSS should probably be updated to reflect that as well.

/**** Mobile-only CSS ****/
@media (max-width: 660px) and (max-height: 660px) { } 

@ongsalt
Copy link
Author

ongsalt commented May 13, 2023

As for screen height. There are a lot of place where @media (max-width: 660px) is use. I don't know if changing any of this will break something else.

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.

None yet

2 participants