infrastructure/web-assets
1
4
Fork 3
Code Issues 3 Pull Requests 1 Projects Releases 42 Wiki Activity

Pagination styles for mobile version #94704

Merged
Márton Lente merged 2 commits from Francesco-Bellini/web-assets:fix-mobile-pagination into v2 2024-10-01 10:49:12 +02:00
Conversation 13 Commits 2 Files Changed 2 +98 -3
Francesco Bellini commented 2024-09-25 12:11:26 +02:00
Contributor
Copy Link

Edit

Please, checkout the update comment.

Hi again :)

Today i'm fighting for the mobile pagination rights.

While the pagination looks fine for desktop

immagine

seems like mobile is not so lucky...

immagine

cutting buttons on the left side, and overflowing to the right, causing the whole page to enlarge.

immagine

Proposal

All those buttons are simply too many for a mobile screen.

My proposal is (for mobile only) to split the First, Prev, Next, Last buttons in a separate line (while of course, hiding the desktop ones).

immagine

## Edit Please, checkout the [update comment](https://projects.blender.org/infrastructure/web-assets/pulls/94704#issuecomment-1305236). --- Hi again :) Today i'm fighting for the mobile pagination rights. While the pagination looks fine for desktop ![immagine](/attachments/77e1b259-c6de-4512-8730-c9d964f8bba7) seems like mobile is not so lucky... ![immagine](/attachments/02f077ed-d017-421c-875c-1d60b1152a63) cutting buttons on the left side, and overflowing to the right, causing the whole page to enlarge. ![immagine](/attachments/aa35dfc7-5a71-4316-b7ba-e6c63a78c1ea) --- ### Proposal All those buttons are simply too many for a mobile screen. My proposal is (for mobile only) to split the `First`, `Prev`, `Next`, `Last` buttons in a separate line (while of course, hiding the desktop ones). ![immagine](/attachments/43c1aa51-3b8a-4459-bf2b-d9d280af3804)
immagine.png
5.9 KiB
immagine.png
2.6 KiB
immagine.png
8.5 KiB
immagine.png
36 KiB
Francesco Bellini added 1 commit 2024-09-25 12:11:28 +02:00
Francesco Bellini commented 2024-09-25 17:06:15 +02:00
Author
Contributor
Copy Link

I'm realizing that hiding from web-assets the desktop pagination buttons, also affect other websites which use web-assets (if they use this same .pagination class).

While each website can point a different version, this can lead to future problems.

I'm not exactly sure which other websites uses this pagination style, but they should implement the same updates as the PR i made on extensions-website, to work properly after updating web-assets. I could try to extend the HTML implementation on those, if you could point me in the right repos that would need that. As long as the solution looks good to you, of course.

Edit:
Looking the websites using v2, i only found (note: quick and unlogged look):

  • https://studio.blender.org/blog/ (which seems to use paginator without pages, so it's affected, but having no pages numbers, probably don't need this update; still would need to be adjusted to avoid hiding buttons on mobile)
    For example, adding a unique class there, at the .pagination level, and use that here, with :not(), while hiding on mobile, could be one solution to exclude studio.blender.org custom pagination.
    Or the opposite, adding the class in extensions-website and using it to hide the buttons here (instead of children of .pagination), which probably makes more sense and is more easy to maintain, enabling the mobile styling only where needed.
I'm realizing that hiding from web-assets the desktop pagination buttons, also affect other websites which use web-assets (if they use this same .pagination class). While each website can point a different version, this can lead to future problems. I'm not exactly sure which other websites uses this pagination style, but they should implement the same updates as the [PR](https://projects.blender.org/infrastructure/extensions-website/pulls/255) i made on extensions-website, to work properly after updating web-assets. I could try to extend the HTML implementation on those, if you could point me in the right repos that would need that. As long as the solution looks good to you, of course. Edit: Looking the websites using v2, i only found (note: quick and unlogged look): - https://studio.blender.org/blog/ (which seems to use paginator without pages, so it's affected, but having no pages numbers, probably don't need this update; still would need to be adjusted to avoid hiding buttons on mobile) *For example, adding a unique class there, at the .pagination level, and use that here, with :not(), while hiding on mobile, could be one solution to exclude studio.blender.org custom pagination. Or the opposite, adding the class in extensions-website and using it to hide the buttons here (instead of children of .pagination), which probably makes more sense and is more easy to maintain, enabling the mobile styling only where needed.*
Márton Lente was assigned by Pablo Vazquez 2024-09-27 15:52:43 +02:00
Pablo Vazquez commented 2024-09-27 15:53:13 +02:00
Owner
Copy Link

Thank you Francesco! I've assigned Márton to have a look at it.

Thank you Francesco! I've assigned Márton to have a look at it.
👍 1
Márton Lente commented 2024-09-27 17:05:16 +02:00
Owner
Copy Link

Hi @Francesco-Bellini , Thank you for your proposal again! Addressing this issue is indeed useful, and the suggestions look good to me in general. I'll add some generic comments here, and some minor, specific change requests below to align better with the system.

  • Adding a 'duplicate' / extra tag for the nav to be displayed in a specific media query is not recommended semantically, but here it does make sense because of achieving these layouts would otherwise be difficult. If you have a better idea to resolve this without introducing a new tag (e.g. by changing items' order with order CSS property per query etc.) let us know, otherwise looks ok as is.
  • If possible, adding a markup example to Web Assets reference page would be nice, so that we can see how the component markup is meant to look. This would make the PR more compact and complete, and also easier to review.

Thank you!

Hi @Francesco-Bellini , Thank you for your proposal again! Addressing this issue is indeed useful, and the suggestions look good to me in general. I'll add some generic comments here, and some minor, specific change requests below to align better with the system. - Adding a 'duplicate' / extra tag for the nav to be displayed in a specific media query is not recommended semantically, but here it does make sense because of achieving these layouts would otherwise be difficult. If you have a better idea to resolve this without introducing a new tag (e.g. by changing items' order with `order` CSS property per query etc.) let us know, otherwise looks ok as is. - If possible, adding a markup example to Web Assets [reference page](https://projects.blender.org/infrastructure/web-assets/src/branch/v2/src/index.html) would be nice, so that we can see how the component markup is meant to look. This would make the PR more compact and complete, and also easier to review. Thank you!
Francesco Bellini commented 2024-09-27 17:19:29 +02:00
Author
Contributor
Copy Link

Hi @martonlente!

Sure! I didn't noticed that this repo had a reference page with examples. I will add it for sure!

This evening i'll be back on my linux system, and will work on that.

Also i will dig further... Maybe i will be able to do something requiring no (or less) html changes on the website side.

In case i couldn't find another solution, what do you think about the last part of my comment?

Do you agree it would be better to add a unique class, along with .pagination, in the extensions-website, so here, when i hide the desktop button, i can use that, instead of .pagination, so that studio.blender.org (and eventual other i missed) are not affected, if not explicitly implemented for it?

Hi @martonlente! Sure! I didn't noticed that this repo had a reference page with examples. I will add it for sure! This evening i'll be back on my linux system, and will work on that. Also i will dig further... Maybe i will be able to do something requiring no (or less) html changes on the website side. --- _In case i couldn't find another solution_, what do you think about the last part of my comment? Do you agree it would be better to add a unique class, along with .pagination, in the extensions-website, so here, when i hide the desktop button, i can use that, instead of .pagination, so that studio.blender.org (and eventual other i missed) are not affected, if not explicitly implemented for it?
👍 1
Márton Lente requested changes 2024-09-27 17:42:40 +02:00
src/styles/_pagination.sass Outdated
@ -1,4 +1,4 @@
.pagination
.pagination, .pagination-m
Márton Lente commented 2024-09-27 17:19:02 +02:00
Owner
Copy Link

Web Assets styles are mobile-first by default. As such, it'd make sense to make mobile / small screen styling kick in by default, and adding additional styles for larger screens instead.

Here it's acceptable like this too. I suggest changing pagination-m to pagination-xs to reflect the project's naming scheme.

Web Assets styles are mobile-first by default. As such, it'd make sense to make mobile / small screen styling kick in by default, and adding additional styles for larger screens instead. Here it's acceptable like this too. I suggest changing `pagination-m` to `pagination-xs` to reflect the project's naming scheme.
👍 1
Francesco Bellini commented 2024-09-28 17:04:48 +02:00
Author
Contributor
Copy Link

Removed the .pagination-m class at all.

Removed the `.pagination-m` class at all.
Francesco-Bellini marked this conversation as resolved
src/styles/_pagination.sass Outdated
@ -15,2 +15,4 @@
+padding(0, x)
.pagination
@media (max-width: 767px)
Márton Lente commented 2024-09-27 17:20:53 +02:00
Owner
Copy Link

We shouldn't use explicit px values in media queries, but instead use media mixins bundled with the project. I think +media-xs would qualify here.

We shouldn't use explicit px values in media queries, but instead use media mixins bundled with the project. I think `+media-xs` would qualify here.
👍 1
Francesco-Bellini marked this conversation as resolved
src/styles/_pagination.sass Outdated
@ -17,0 +21,4 @@
.pagination-m
display: none
+padding(1, y)
Márton Lente commented 2024-09-27 17:23:28 +02:00
Owner
Copy Link

I'd suggest matching this with the existing pagination's vertical spacing setting to +padding(2, y), so that no utility classes are needed in projects when we implement this, if we want to keep the existing spacings.

I'd suggest matching this with the existing `pagination`'s vertical spacing setting to `+padding(2, y)`, so that no utility classes are needed in projects when we implement this, if we want to keep the existing spacings.
Márton Lente commented 2024-09-27 17:45:06 +02:00
Owner
Copy Link

Or actually +padding(2, bottom) would be cleaner, as pagination comes before pagination-m in the markup with bottom spacings already applied.

Or actually `+padding(2, bottom)` would be cleaner, as `pagination` comes before `pagination-m` in the markup with bottom spacings already applied.
Francesco Bellini commented 2024-09-28 17:07:52 +02:00
Author
Contributor
Copy Link

See the new comment for details, because changing overall strategy, there was new requirements.

See the new [comment](https://projects.blender.org/infrastructure/web-assets/pulls/94704#issuecomment-1305236) for details, because changing overall strategy, there was new requirements.
👍 1
Francesco-Bellini marked this conversation as resolved
Márton Lente commented 2024-09-27 18:01:17 +02:00
Owner
Copy Link

I'm realizing that hiding from web-assets the desktop pagination buttons, also affect other websites which use web-assets (if they use this same .pagination class).

While each website can point a different version, this can lead to future problems.

I'm not exactly sure which other websites uses this pagination style, but they should implement the same updates as the PR i made on extensions-website, to work properly after updating web-assets. I could try to extend the HTML implementation on those, if you could point me in the right repos that would need that. As long as the solution looks good to you, of course.

Edit:
Looking the websites using v2, i only found (note: quick and unlogged look):

  • https://studio.blender.org/blog/ (which seems to use paginator without pages, so it's affected, but having no pages numbers, probably don't need this update; still would need to be adjusted to avoid hiding buttons on mobile)
    For example, adding a unique class there, at the .pagination level, and use that here, with :not(), while hiding on mobile, could be one solution to exclude studio.blender.org custom pagination.
    Or the opposite, adding the class in extensions-website and using it to hide the buttons here (instead of children of .pagination), which probably makes more sense and is more easy to maintain, enabling the mobile styling only where needed.

Thanks for looking out for this. Since the v2 branch is in alpha yet, I think it's ok to come up with a solution for this that we find the best, even if it needs some (moderate) adjustments on the projects' level.

> I'm realizing that hiding from web-assets the desktop pagination buttons, also affect other websites which use web-assets (if they use this same .pagination class). > > While each website can point a different version, this can lead to future problems. > > I'm not exactly sure which other websites uses this pagination style, but they should implement the same updates as the [PR](https://projects.blender.org/infrastructure/extensions-website/pulls/255) i made on extensions-website, to work properly after updating web-assets. I could try to extend the HTML implementation on those, if you could point me in the right repos that would need that. As long as the solution looks good to you, of course. > > Edit: > Looking the websites using v2, i only found (note: quick and unlogged look): > > - https://studio.blender.org/blog/ (which seems to use paginator without pages, so it's affected, but having no pages numbers, probably don't need this update; still would need to be adjusted to avoid hiding buttons on mobile) > *For example, adding a unique class there, at the .pagination level, and use that here, with :not(), while hiding on mobile, could be one solution to exclude studio.blender.org custom pagination. > Or the opposite, adding the class in extensions-website and using it to hide the buttons here (instead of children of .pagination), which probably makes more sense and is more easy to maintain, enabling the mobile styling only where needed.* > Thanks for looking out for this. Since the `v2` branch is in _alpha_ yet, I think it's ok to come up with a solution for this that we find the best, even if it needs some (moderate) adjustments on the projects' level.
Francesco Bellini force-pushed fix-mobile-pagination from 4f8bb162ad to dbe6a20cee 2024-09-28 16:14:19 +02:00 Compare
Francesco Bellini commented 2024-09-28 16:53:46 +02:00
Author
Contributor
Copy Link

Adding a 'duplicate' / extra tag for the nav to be displayed in a specific media query is not recommended semantically, but here it does make sense because of achieving these layouts would otherwise be difficult. If you have a better idea to resolve this without introducing a new tag (e.g. by changing items' order with order CSS property per query etc.) let us know, otherwise looks ok as is.

I'm glad you asked me to try to find another solution, @martonlente, cause i did, and it's good to know!

I found a solution requiring no changes in the website side.

And updated the reference page as requested.

Updates

  • Changed the media query to +media-xs

  • Removed the pagination-m class

  • Add flex-wrap: wrap to .pagination

  • Moved 1 unit of padding top from the .pagination to 1 unit of margin top in the .page-link
    For wrapping reasons. No visible changes.

  • Used order, as suggested to reorder to end special buttons on small screens (while preserving relative order)

  • For small screen, used a .pagination::before in order to force a line between pages and special buttons (using order)
    This is because heavy paginations can have pages wrapping, and they must not mix with the line of the special buttons.

  • Created optional class .pagination-inline, which can be used dinamically on website side (based on context;number of total buttons), in order to force inline mode for small screens too.
    As mentioned in the last pagination example in the reference page.
    I tought it was nice to have as a possibility, but if you don't see a use for it, i can remove it.
    I'm guessing the pagination on studio.blender.org i've seen, could benefit by using a static pagination-inline class, since it has no pages, so has a short row, but should be tested to make sure.

  • Updated reference page, adding 2 examples (see images next):

    1. Heavy pagination (to have a look on worst case scenario, and see how it works on small screens)
    2. Example of a situation with few buttons, using the class pagination-inline

  • Also in the reference page i added the left class to the first example, because the text was saing so, but the class wasn't there.

  • I removed the box around pagination examples (as tables), in order to get the tipical width on small screens (like extensions-website) without having the extra padding from the box.

I tested both from local extensions-website as from reference-page.

Here the reference page with normal and small screens:

image

image

P.s. I was wrong about the worst possibile case. This heavy example is the actual worst possible case (at least, for extensions-website)... (being in a far central page in a ~15+ pages situation, which gives 2 "..." and 5 pages on each side of the active one, for a total of 17 buttons on the screen)

For any changes, let me know!

If you agree on the solution, i will close infrastructure/extensions-website#255, as this task doesn't need HTML changes anymore, and given the number of extensions in production, extensions-website seems like will never need to use pagination-inline optional implementation.

P.s. I amended previous commit and rebased, but i've got a backup branch of the initial PR.

> Adding a 'duplicate' / extra tag for the nav to be displayed in a specific media query is not recommended semantically, but here it does make sense because of achieving these layouts would otherwise be difficult. If you have a better idea to resolve this without introducing a new tag (e.g. by changing items' order with order CSS property per query etc.) let us know, otherwise looks ok as is. I'm glad you asked me to try to find another solution, @martonlente, cause i did, and it's good to know! I found a solution requiring no changes in the website side. And updated the reference page as requested. --- ### Updates - Changed the media query to `+media-xs` - Removed the `pagination-m` class - Add `flex-wrap: wrap` to `.pagination` - Moved 1 unit of padding top from the `.pagination` to 1 unit of margin top in the `.page-link` *For wrapping reasons. No visible changes.* - Used order, as suggested to reorder to end special buttons on small screens (while preserving relative order) - For small screen, used a `.pagination::before` in order to force a line between pages and special buttons (using order) *This is because heavy paginations can have pages wrapping, and they must not mix with the line of the special buttons.* - Created optional class `.pagination-inline`, which can be used dinamically on website side (based on context;number of total buttons), in order to force inline mode for small screens too. *As mentioned in the last pagination example in the reference page. I tought it was nice to have as a possibility, but if you don't see a use for it, i can remove it.* _I'm guessing the pagination on studio.blender.org i've seen, could benefit by using a static `pagination-inline` class, since it has no pages, so has a short row, but should be tested to make sure._ - Updated reference page, adding 2 examples (see images next): 1. Heavy pagination (to have a look on worst case scenario, and see how it works on small screens) 2. Example of a situation with few buttons, using the class `pagination-inline` - Also in the reference page i added the `left` class to the first example, because the text was saing so, but the class wasn't there. - I removed the box around pagination examples (as tables), in order to get the tipical width on small screens (like extensions-website) without having the extra padding from the box. --- I tested both from local extensions-website as from reference-page. Here the reference page with normal and small screens: ![image](/attachments/9a1b7c33-b343-47d7-85f6-b32c2f0c9c63) ![image](/attachments/46537bef-8925-4cd4-a061-6c4dc3ac7f39) _P.s. I was wrong about the worst possibile case. This heavy example is the actual worst possible case (at least, for extensions-website)... (being in a far central page in a ~15+ pages situation, which gives 2 "..." and 5 pages on each side of the active one, for a total of 17 buttons on the screen)_ --- For any changes, let me know! If you agree on the solution, i will close infrastructure/extensions-website#255, as this task doesn't need HTML changes anymore, and given the number of extensions in production, extensions-website seems like will never need to use `pagination-inline` optional implementation. P.s. I amended previous commit and rebased, but i've got a [backup branch](https://projects.blender.org/Francesco-Bellini/web-assets/src/branch/backup-fix-mobile-pagination-v1) of the initial PR.
image.png
82 KiB
image.png
61 KiB
❤️ 2
Francesco Bellini force-pushed fix-mobile-pagination from dbe6a20cee to 20ca35103c 2024-09-28 17:18:24 +02:00 Compare
Francesco Bellini requested review from Márton Lente 2024-09-28 17:20:30 +02:00
Francesco Bellini force-pushed fix-mobile-pagination from 20ca35103c to 0f1386a086 2024-09-28 17:24:49 +02:00 Compare
Francesco Bellini force-pushed fix-mobile-pagination from 0f1386a086 to 75258cfe2c 2024-09-29 14:50:51 +02:00 Compare
Francesco Bellini force-pushed fix-mobile-pagination from 75258cfe2c to 4ec83f199e 2024-09-29 15:00:08 +02:00 Compare
Márton Lente commented 2024-09-30 16:12:54 +02:00
Owner
Copy Link

@Francesco-Bellini thank you for the changes and the thorough description, they look very good! I also appreciate you managed the changes in a way that they don't require markup changes – that's nice!

Before merging, just let me add some minor change requests or comments below.

Heavy pagination (to have a look on worst case scenario, and see how it works on small screens)

I think later we might want to simplify this, as these many buttons indeed seem to be a bit too much from a UX perspective. Though a similar component is currently used in Extensions, so I think it's good to have this polished example in Web Assets for now.

Created optional class .pagination-inline, which can be used dinamically on website side (based on context;number of total buttons), in order to force inline mode for small screens too.

I'm not sure about the use case, but the naming and styles are nice and clean: I'm okay with this.

@Francesco-Bellini thank you for the changes and the thorough description, they look very good! I also appreciate you managed the changes in a way that they don't require markup changes – that's nice! Before merging, just let me add some minor change requests or comments below. > Heavy pagination (to have a look on worst case scenario, and see how it works on small screens) I think later we might want to simplify this, as these many buttons indeed seem to be a bit too much from a UX perspective. Though a similar component is currently used in Extensions, so I think it's good to have this polished example in Web Assets for now. > Created optional class .pagination-inline, which can be used dinamically on website side (based on context;number of total buttons), in order to force inline mode for small screens too. I'm not sure about the use case, but the naming and styles are nice and clean: I'm okay with this.
Márton Lente requested changes 2024-09-30 16:22:43 +02:00
src/index.html Outdated
@ -1302,3 +1301,3 @@
</p>
<ul class="pagination">
<ul class="pagination left">
Márton Lente commented 2024-09-30 16:17:48 +02:00
Owner
Copy Link

I suggest not to change the default aligment on the reference page.

Not related to the PR, but I'd prefer BEM-like naming, like .pagination-left instead of a generic nested .left for the class selector.

I suggest not to change the default aligment on the reference page. Not related to the PR, but I'd prefer BEM-like naming, like `.pagination-left` instead of a generic nested `.left` for the class selector.
Francesco Bellini commented 2024-09-30 16:38:59 +02:00
Author
Contributor
Copy Link

I changed this (unrelated example) only because it's description was saying:

Left aligned pagination example

immagine

But there wasn't no left class. So the example wasn't aligned as the description was saying.
And using the class left is what the next example suggest.

I can undo this, if i'm missing something here. I just tought it was forgotten...

I changed this (unrelated example) only because it's description was saying: `Left aligned pagination example` ![immagine](/attachments/d912a832-fa17-4f61-8fde-e34d22c98817) But there wasn't no `left` class. So the example wasn't aligned as the description was saying. And using the class `left` is what the next example suggest. I can undo this, if i'm missing something here. I just tought it was forgotten...
immagine.png
9.3 KiB
Márton Lente commented 2024-09-30 20:33:13 +02:00
Owner
Copy Link

Thank you for your attention to detail. I'd personally recommend undoing this, to keep the PR focused and the pagination example section more relaxed. If we really want to address this, I think it'd be better to change not the markup but the text by removing the Left aligned pagination example. part, that isn't very useful here.

The reference page is a work in progress, and maybe it's better to do another 'cleanup and improvement' pass on its layout and text in another pass.

Thank you for your attention to detail. I'd personally recommend undoing this, to keep the PR focused and the pagination example section more relaxed. If we really want to address this, I think it'd be better to change not the markup but the text by removing the _<del>Left aligned pagination example.</del>_ part, that isn't very useful here. The reference page is a work in progress, and maybe it's better to do another 'cleanup and improvement' pass on its layout and text in another pass.
Francesco Bellini commented 2024-09-30 22:56:35 +02:00
Author
Contributor
Copy Link

Fear :)

I'll undo this.

Fear :) I'll undo this.
Francesco-Bellini marked this conversation as resolved
src/styles/_pagination.sass Outdated
@ -17,0 +22,4 @@
order: 2
.pagination:not(.pagination-inline)::before
order: 1
Márton Lente commented 2024-09-30 16:21:07 +02:00
Owner
Copy Link

I'd prefer not to use the :not selector if possible, to make the base pagination styles work also if we remove the extra .pagination-inline later, just in case.

I'd prefer not to use the `:not` selector if possible, to make the base pagination styles work also if we remove the extra `.pagination-inline` later, just in case.
Francesco Bellini commented 2024-09-30 16:43:22 +02:00
Author
Contributor
Copy Link

:not() here, is only making sure that when using pagination-inline class, this style does not kick in.

Basically the class pagination-inline doesn't add nothing new.
It just remove this styling when used (on purpose).

I made an example of using this class, in a new comment. Let me know :D

:not() here, is only making sure that when using `pagination-inline` class, this style does not kick in. Basically the class `pagination-inline` doesn't add nothing new. It just remove this styling when used (on purpose). I made an example of using this class, in a new comment. Let me know :D
Márton Lente commented 2024-09-30 20:50:25 +02:00
Owner
Copy Link

I see how you're thinking, and indeed it's correct and works well. But in a BEM-like convention that we try to apply in the framework it would rather look like this:

+media-xs
  .pagination
    .page-first, .page-prev, .page-next, .page-last
      order: 2

    &::before
      order: 1
      content: ""
      width: 100%

    &.pagination-inline
      .page-first, .page-prev, .page-next, .page-last
        order: 0

      &::before
        display: none

Even if it results in more code, it's cleaner to start with the base selector styles, and add extra styles for the extra modifiers / variants, that can be easily removed without touching the base selectors and styles, just in case. I hope this explains what I mean here, and suggest to use this instead. It also has a performance benefit (that's mostly theoretical in this case, of course).

I see how you're thinking, and indeed it's correct and works well. But in a BEM-like convention that we try to apply in the framework it would rather look like this: ``` +media-xs .pagination .page-first, .page-prev, .page-next, .page-last order: 2 &::before order: 1 content: "" width: 100% &.pagination-inline .page-first, .page-prev, .page-next, .page-last order: 0 &::before display: none ``` Even if it results in more code, it's cleaner to start with the base selector styles, and add extra styles for the extra modifiers / variants, that can be easily removed without touching the base selectors and styles, just in case. I hope this explains what I mean here, and suggest to use this instead. It also has a performance benefit (that's mostly theoretical in this case, of course).
Francesco Bellini commented 2024-09-30 23:02:31 +02:00
Author
Contributor
Copy Link

I never say no to the "mostly theoretical" performance benefits!

And i'm not quite familiar with this BEM-like convention you're referring, but i'll check it out to better understand the basic concepts.

Thanks for the snippet! I'm going to update the PR this way 👍

I never say no to the "mostly theoretical" performance benefits! And i'm not quite familiar with this BEM-like convention you're referring, but i'll check it out to better understand the basic concepts. Thanks for the snippet! I'm going to update the PR this way 👍
❤️ 1
Francesco-Bellini marked this conversation as resolved
src/styles/_pagination.sass Outdated
@ -18,3 +30,4 @@
color: var(--color-text-secondary)
font-size: var(--fs-sm)
line-height: var(--lh-base)
order: 0
Márton Lente commented 2024-09-30 16:22:31 +02:00
Owner
Copy Link

Are you sure this order rule is needed? The order should default to 0, if not defined.

Are you sure this `order` rule is needed? The `order` should default to `0`, if not defined.
Francesco Bellini commented 2024-09-30 16:45:27 +02:00
Author
Contributor
Copy Link

Yes, I think you're right... I'll test it and eventually remove it.

I've add it on principle, to be honest, so i didn't tried without it.

Yes, I think you're right... I'll test it and eventually remove it. I've add it on principle, to be honest, so i didn't tried without it.
Márton Lente commented 2024-09-30 20:52:04 +02:00
Owner
Copy Link

I tested this and it's safe to remove order: 0, as elements are rendered like this by default.

I tested this and it's safe to remove `order: 0`, as elements are rendered like this by default.
Francesco Bellini commented 2024-09-30 23:03:24 +02:00
Author
Contributor
Copy Link

Thanks for checking this out!

Happy to remove it, then!

Thanks for checking this out! Happy to remove it, then!
Francesco-Bellini marked this conversation as resolved
Francesco Bellini commented 2024-09-30 16:34:52 +02:00
Author
Contributor
Copy Link

I think later we might want to simplify this, as these many buttons indeed seem to be a bit too much from a UX perspective. Though a similar component is currently used in Extensions, so I think it's good to have this polished example in Web Assets for now.

Yes, i agree it would be better to reduce the number of max buttons, by design on extensions-website... Because only the pages can raise up to 13 buttons...

For the pagination-inline class, here's a use-case:

This is what would happend to the studio.blender.org/blog pagination, if we do nothing (after this PR):

I quickly replicated from devtools...
immagine

But as you can see here, the studio.blender.org/blog pagination, has no pages...
So, on first and last page, it's perfectly fine without wrapping the buttons, also on small screen.

immagine

My idea behind pagination-inline was simple:

If the pagination is short enought, we can keep it inline.

For example, in studio.blender.org, we could check if the page is 1 or the last, and in that case, add the class pagination-inline.
Because those are the only 2 cases having exeeding space (because first and last pages only has 2 buttons).

While other cases like this:

immagine

would require to have a separate row.

Of course, if you are fine having the media query always kicking in, regardless the number of buttons, i can remove it.

> I think later we might want to simplify this, as these many buttons indeed seem to be a bit too much from a UX perspective. Though a similar component is currently used in Extensions, so I think it's good to have this polished example in Web Assets for now. Yes, i agree it would be better to reduce the number of max buttons, by design on extensions-website... Because only the pages can raise up to 13 buttons... --- For the `pagination-inline` class, here's a use-case: This is what would happend to the studio.blender.org/blog pagination, if we do nothing (after this PR): *I quickly replicated from devtools...* ![immagine](/attachments/eecdafe5-f516-43de-84f7-671ce1771729) But as you can see here, the studio.blender.org/blog pagination, has no pages... So, on first and last page, it's perfectly fine without wrapping the buttons, also on small screen. ![immagine](/attachments/0df2f206-be0d-460e-8b9e-40bd9aff88f9) --- My idea behind `pagination-inline` was simple: **_If the pagination is short enought, we can keep it inline._** --- For example, in studio.blender.org, we could check if the page is `1` or the `last`, and in that case, add the class `pagination-inline`. Because those are the only 2 cases having exeeding space (because first and last pages only has 2 buttons). While other cases like this: ![immagine](/attachments/6ca9902a-d332-40ba-94d3-8d16968c5625) would require to have a separate row. --- Of course, if you are fine having the media query always kicking in, regardless the number of buttons, i can remove it.
immagine.png
18 KiB
immagine.png
18 KiB
immagine.png
16 KiB
immagine.png
16 KiB
immagine.png
Márton Lente commented 2024-09-30 20:54:01 +02:00
Owner
Copy Link

For the pagination-inline class, here's a use-case:

I'm convinced, let's keep the new pagination-inline styles! 👍

Thank you for all your work on this, I think we're almost there!

> For the `pagination-inline` class, here's a use-case: I'm convinced, let's keep the new `pagination-inline` styles! 👍 Thank you for all your work on this, I think we're almost there!
❤️ 1
Francesco Bellini force-pushed fix-mobile-pagination from 4ec83f199e to 85cc564669 2024-09-30 23:54:03 +02:00 Compare
Francesco Bellini commented 2024-09-30 23:59:39 +02:00
Author
Contributor
Copy Link

There we go!

  • Undo left class in reference page
  • Avoided :not(...) as suggested

infrastructure/extensions-website#255 closed.

There we go! - Undo `left` class in reference page - Avoided `:not(...)` as [suggested](https://projects.blender.org/infrastructure/web-assets/pulls/94704#issuecomment-1306908) --- infrastructure/extensions-website#255 closed.
❤️ 1 🚀 1
Francesco Bellini requested review from Márton Lente 2024-09-30 23:59:47 +02:00
Márton Lente merged commit 847b5e8992 into v2 2024-10-01 10:49:12 +02:00
Márton Lente commented 2024-10-01 11:00:44 +02:00
Owner
Copy Link

@Francesco-Bellini your PR was merged to main, and will get deployed to projects with the next release.

Thank you for the nice fixes! 👍

@Francesco-Bellini your PR was merged to `main`, and will get deployed to projects with the next release. Thank you for the nice fixes! 👍
👍 1
Pablo Vazquez commented 2024-10-01 11:35:11 +02:00
Owner
Copy Link

Thanks for the help @Francesco-Bellini ! Mobile rights need lots of love in Blender projects, it's always a struggle.

And thanks Márton for reviewing!

Thanks for the help @Francesco-Bellini ! Mobile rights need lots of love in Blender projects, it's always a struggle. And thanks Márton for reviewing!
👍 2
Pablo Vazquez deleted branch fix-mobile-pagination 2024-10-01 11:35:22 +02:00
Francesco Bellini commented 2024-10-01 12:14:25 +02:00
Author
Contributor
Copy Link

Thank you guys (@martonlente @pablovazquez)!
I enjoy working with you, and i have deep admiration for the Blender project as a whole. To me, it's #1 FOSS project out there.

That's why i'm very happy to help wherever i can.

P.s. I don't mean to bother/rush you in any way, and maybe it's just a very low priority for you, but i noticed that you guys are not subscribed to extensions-website, so maybe you just missed that. Just wanted to mention that i have a 1 CSS line PR there, which should be very quick to handle.

Hope i'll meet you guys at the bcon!

Thank you guys (@martonlente @pablovazquez)! I enjoy working with you, and i have deep admiration for the Blender project as a whole. To me, it's #1 FOSS project out there. That's why i'm very happy to help wherever i can. > P.s. I don't mean to bother/rush you in any way, and maybe it's just a very low priority for you, but i noticed that you guys are not subscribed to extensions-website, so maybe you just missed that. Just wanted to mention that i have a 1 CSS line PR there, which should be very quick to handle. --- Hope i'll meet you guys at the bcon!
❤️ 2
Sign in to join this conversation.
Reviewers
No reviewers
Márton Lente
Labels
Clear labels   
legacy project
Infrastructure: Blender Web Assets

  
legacy project
Infrastructure: Websites

  
Priority
High

  
Priority
Low

  
Priority
Normal

  
Status
Archived

  
Status
Resolved

  
Type
Design

  
Type
Report

  
Type
To Do

No Label
legacy project
Infrastructure: Blender Web Assets
legacy project
Infrastructure: Websites
Priority
High
Priority
Low
Priority
Normal
Status
Archived
Status
Resolved
Type
Design
Type
Report
Type
To Do
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
Márton Lente
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: infrastructure/web-assets#94704
No description provided.
Delete Branch "Francesco-Bellini/web-assets:fix-mobile-pagination"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?