-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Make the old modal system responsive and always centred without JS #23693
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
Conversation
| // sometimes the modal can be displayed outside of the current viewport, in this case | ||
| // we scroll to it to make sure it's visible. this isn't a perfect workaround, since it | ||
| // doesn't center the modal.g | ||
| var self = this; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ Variable not used. That was already he case before my development.
| var centerPopover = function () { | ||
| if (container !== false) { | ||
| $('.ui-dialog').css({margin: '0 0'}); | ||
| container.dialog("option", "position", {my: 'center', at: 'center', of: '.ui-widget-overlay', collision: 'fit'}); | ||
| // in some cases jQuery UI fails to place the dialog correctly and set the top values to something negative | ||
| if ($('.ui-dialog').position().top < 0) { | ||
| $('.ui-dialog').css('top', '0'); | ||
| } | ||
| $('.ui-dialog').css({margin: '15px 0'}); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ I was able to keep a perfect centering in pure CSS, so I removed this calculation. Since the width/height measurement is blocking the browser, it is a performance gain.
|
|
||
| @media (max-width: 600px) { | ||
| padding: 15px; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ Remove the left padding that was here to display the icon. Make it smaller in the same time
| display: block; | ||
| margin-bottom: 0.5em; | ||
| text-align: center; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ Make the icon on top instead on the left of the message on mobile. It will take less space.
| @media (max-width: 600px) { | ||
| position: static; | ||
| display: block; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| z-index: 1001; | ||
| inset: 15px !important; | ||
| margin: auto; | ||
| width: min-content; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ Tag Manager modals do not have explicit width defined with style attribute.
So we are setting a default width here, to keep a small modal in these cases.
cfeda93 to
e463739
Compare
937cb7b to
6f2ed13
Compare
6009781 to
467cc66
Compare
plugins/Live/stylesheets/live.less
Outdated
| ol.actionList > li.collapsed-content-item::after { | ||
| border-right: 2px solid #d2d2d2; | ||
| width: 26px; | ||
| left: -34px; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cb9acb0 to
668a4cf
Compare
| const $triggerItem = $(e.target).closest('li'); | ||
| const $root = $triggerItem.parent(); | ||
| const actionsToDisplayCollapsed = +piwik.visitorLogActionsToDisplayCollapsed; | ||
| const isExpanded = $triggerItem.toggleClass('expanded collapsed').hasClass('expanded'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ One time, bu non-reproductible, I’ve got the link label messed up.
By having an explicit state here, I assure that everything is always consistent.
| }); | ||
| const currentIndex = $actions.index(this); | ||
| if (currentIndex === actionsToDisplayCollapsed - 1) { | ||
| $(this).toggleClass('last-collapsed-action', !isExpanded); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ Adding this to avoid having a thread that goes nowhere after the last visible item when an action list is collapsed.
| {% set previousAction = false %} | ||
| {% for actionGroup in actionGroups %} | ||
| {% if actionGroup.pageviewAction is not empty %} | ||
| {{ postEvent('Live.renderAction', actionGroup.pageviewAction, previousAction, visitInfo) }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ This file contains only whitespace (indentation / extra lines) refactoring because I couldn’t follow.
72bcefd to
199151f
Compare
| $li.addClass('last-collapsed-content'); | ||
| $li.prev().removeClass('last-collapsed-content'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ Small clean up to have an explicit code that add a class on the last collapsed content.
michalkleiner
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for all the additional changes and the adjustments to the timelines 👍




Description:
The old modal design had 2 big responsive design:
Plugin/Submodule update
2 PRs are open in plugin/submodule to update UI screenshots:
Visits log | desktop
ℹ️ Better vertical centering, and more intuitive scroll
Visits log | mobile
ℹ️ No content outside of the screen.
Performance | desktop
ℹ️ Better vertical centering.
Performance | mobile
ℹ️ Vertical centering always up to date
Row evolution | desktop
ℹ️ No hidden content.
Row evolution | mobile
ℹ️ Vertical centering always up to date
Profile modale
ℹ️ Vertical centering always up to date
The 2-columns layout is now responsive
I had to make a lot of small fixes. For example, action liste used
inline-blockelements as wrapper that made it hard to control the children width. By fixing it, it changed all actions list inside the app, fixing bugs and spacing in all cases. For example:Checklist
Review