Closed rowild closed 4 years ago
The magnifier glass to show the search input field should be enough. Is that PR independent from the first one?
It was intended to be independent, but 2 or 3 overlaps happened, I believe, simply, because the addition of the new wrappers affects sidebar and main content area... But eventually it should be possible to work on those 2 topics independently.
removed chevron from search icon toggle and adapted its width accordingly. Maybe I should add a new PR that combines the 2 previous ones. Would that help? I also should mention that for the moment these changes are only tested on Chrome and only for mobile state, no tablet state layout yet...
No, two PRs are better than one.
Regarding the "number of products" selector: It looks a bit strange on the left side. You can leave it on the right side and change the Bootstrap class (theres a left and right drop-down)
The positioning of the "number of products per page" selector is done via popper, if I am not mistaken. And that one checks, if a class "dropdown-menu-right" is available or not. Only then the selector is placed right: 0; left: auto;
.
I only can think of a JS solution to add that class dynamically (and it would need a media query listener, all in all to much for such a little thingy, IMO). Can you think of a different solution? (I am not very familiar with neither bootstrap nor popper...)
Alternatively the selector could be placed right-aligned always (by adding that class) and set to a smaller width:
.page-limit .dropdown-menu {
min-width: 4rem;
}
Mobile:
Desktop:
Just change
<ul class="dropdown-menu">
to
<ul class="dropdown-menu dropdown-menu-right">
and everything is fine or not?
But that moves the dropdown to right-aligned on desktop, too. Which is not the case currently and makes sense.
Current desktop layout:
To position the dropdown right-aligned, the class dropdown-menu-right
should only be added in mobile state, which would only be possible with js...
That's fine, it can be aligned right on the desktop too. There's no need for a JS solution. Keep it simple :-)
All right! Will do!
Unfortunately, there are still two merge conflicts left. Can you fix them? They are in the admin/jqadm/themes/admin.js
file.
Hmmm... hope I did that right. The problem exists, because one function actually belongs to the "sidebar-nav" PR, and the second one to this...
In any case: please don't merge yet, there is more to be fixed!
Actually, I think all changes are there. This PR should be complete now.
Ah! Good point! The problem here is, that, when using fixed positioning, the width of scrollbars is ignored, which leads to different result on mobile devices and desktop browsers:
On desktops, when in mobile state, the right align is messed up:
...while being totally fine on mobile devices:
If right: 32px
would be used, then desktops would be ok (unless scrollbars are hidden automatically, see Safari), but the gap to the right would be too large on mobile devices...
Didn't find a way to solve that yet. Any ideas?
We could use absolute instead of fixed for the search field...
EDIT: yes, that is definitely more consistent. And since the toggle scrolls off screen anyway, why not the search field, too... Would that be ok with you?
Relative could be a good option too or try to solve it without positioning and use display: block
only. The div
with the search fields is now always placed below and should not be visible all the time because on mobile devices, this limits the space for the rest of the content.
Not sure, how you imagine relative
to work... Relative positioning would actually indeed leave additional space (which is only removed by absolute or fixed positioning).
Currently, when active, the search field is placed exactly over the pagination component, which is always visible anyway. And it must be placed below the header, otherwise the toggle would be covered and therefore not usable.
May I ask if you tried the solution yet? If not, I could provide a little video...
The suggestion was to avoid overlays and toggle display: none
to display: block
to the search bar is shown between navigation bar and pagination.
Ah, so without animation from the top? So that all the content below (pagination, list of contents) would move up and down, depending on whether the search field is visible or not?
Maybe also with animation but not as overlay. Absolute positioning usually leads to a lot of problems.
Not my preferred solution, but I can do that.
(Moving lots of other contents is usually not recommended; I assume, if somebody really uses 500 items in a list on a slow mobile device, and then wants to open the search field, which needs to move all that stuff down, then the animation will be troublesome... positioning: absolute would be way more performant. Besides that I just realise that the current solution animations "top" instead of using "translate"...)
One more commit with the animation from the top, now displaying correctly on all browsers and devices, only in mobile state (before it was also used for tablet state, but not formatted; however, it does not seem to be necessary for tablet state).
It is the more performant solution compared to the relative positioning, which is, why I fight for it! :-)
Great and thank you very much!
The main content Area in mobile state mainly changes the horizontally scrollable content and, in the case of "product" hide the search field and adds a toggle to make it slide-in/-out from/to the top. Also, the pagination gets smaller and a mobile text variant for the current page is added.
The pagination margin of all its buttons is reduced and it shows a simpler page state text.
The "number of pages per page" selector is moved to the right simply because its dropdown expand to the right, and currently I don't know how to fix that. Personally, I think it is ok to switch the positions of the pagination and the selector... what do you think?
The screenshots have a bit of a different table styling, but that is not implemented (yet).
Is the code formatting better?