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

[4.0] Searchtools mobile display #27443

Merged
merged 6 commits into from
Jan 10, 2020

Conversation

infograf768
Copy link
Member

@infograf768 infograf768 commented Jan 9, 2020

Follow-up on #27416

Summary of Changes

Take off from _toolbar.scss .js-stools-container-selector which interfere with searchtools.css
Correct margins in mobile view as the situation is different when we have .js-stools-container-selector (1 field) or .js-stools-container-selector-first (2 fields).

Testing Instructions

Install multiple languages including Persian (fa-IR)
Display in Mobile view the Installed language manager and also the Menu Items manager.
These contains fields separated from the main searchtools (site/admin for Installed languages and site/admin // menu for menu items.

Patch and use npm

Before patch

LTR

Screen Shot 2020-01-09 at 09 51 41

RTL

Screen Shot 2020-01-09 at 09 55 06

After patch

LTR

Screen Shot 2020-01-09 at 09 12 14

RTL

Screen Shot 2020-01-09 at 09 11 26

@Fedik @brianteeman

@brianteeman
Copy link
Contributor

quickcheck - looks great
will test properly later today

[dir=rtl] & {
margin-right: 0;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as the value is the same for LTR and RTL you should not need to have individual settings

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree. There are other places to also simplify. let's wait for more small changes to modify.

}

@include media-breakpoint-down(xs) {
float: none !important;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already defined under media-breakpoint-down(sm)


@include media-breakpoint-down(xs) {
float: none !important;
width: 100%;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already defined under media-breakpoint-down(sm)

width: 100%;

[dir=ltr] & {
margin-right: 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already defined under media-breakpoint-down(sm)

}

[dir=rtl] & {
margin-right: 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already defined under media-breakpoint-down(sm)

}

@include media-breakpoint-down(xs) {
max-width: 100%;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already defined under media-breakpoint-down(sm)

margin-right: .5rem;

[dir=rtl] & {
margin-right: 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already defined under media-breakpoint-down(sm)

@@ -143,6 +201,15 @@
margin-right: 0;
}
}

@include media-breakpoint-down(xs) {
max-width: 100%;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already defined under media-breakpoint-down(sm)

margin-right: .5rem;

[dir=rtl] & {
margin-right: 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already defined under media-breakpoint-down(sm)

@brianteeman
Copy link
Contributor

It works but it needs cleaning up to prevent repetition

@infograf768
Copy link
Member Author

@ciar4n

Alas I had already merged 8b25305

Therefore your proposals were hard to find out.
Anyway I modified with most of your suggestions.
Except some that kill the alignment in LTR when we have 2 fields (menu items manager) for the second field. I had there to keep repeating margin-right: 0;

Please look again and test. If you propose some more changes, please test them before, following my instructions.

@ciar4n
Copy link
Contributor

ciar4n commented Jan 9, 2020

@infograf768

It looks like you got them all.

On a side note, there is a couple of ways of improving this...

  1. mobile first - only apply the margins to desktops media-breakpoint-up(sm) rather than applying margins to all and then taking it away on small screens.

  2. Create a unique class for all toolbar fields and only define changes once.

@infograf768
Copy link
Member Author

@ciar4n
Not sure at all what you mean as xs may need different margins than sm

We have 3 solutions:

  1. Keep this, test it OK and merge. Then make a new PR.
  2. Kill this PR and someone makes a new PR
  3. Propose a PR to my branch.

@ciar4n
Copy link
Contributor

ciar4n commented Jan 9, 2020

Not sure at all what you mean as xs may need different margins than sm

On a single column layout there is no reason why they should. The frontend template CSS is mobile first, which is why I suggested it here.

@infograf768
Copy link
Member Author

Then please make patch.

@ciar4n
Copy link
Contributor

ciar4n commented Jan 10, 2020

I have tested this item ✅ successfully on 118dc6f

This works. Suggestions can be applied template wide in a separate pr (if required).


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/27443.

@infograf768
Copy link
Member Author

@ciar4n
thanks for testing
BTW, I can demonstrate easily why we will have different margin-right when we are in LTR and 2 fields are concerned. xs needs margin-right: .5rem while sm needs margin-right: 0;
Can send you an animated gif if you desire.

@brianteeman
Please mark your test so that we merge this.

@Fedik
Copy link
Member

Fedik commented Jan 10, 2020

I have tested this item ✅ successfully on 118dc6f


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/27443.

@Quy
Copy link
Contributor

Quy commented Jan 10, 2020

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/27443.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jan 10, 2020
@HLeithner HLeithner merged commit 0b41a3d into joomla:4.0-dev Jan 10, 2020
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jan 10, 2020
@HLeithner
Copy link
Member

Thanks

@HLeithner HLeithner added this to the Joomla 4.0 milestone Jan 10, 2020
@infograf768 infograf768 deleted the 4.0_searchtools_mobile branch January 11, 2020 07:06
brianteeman pushed a commit to brianteeman/joomla-cms that referenced this pull request Feb 4, 2020
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.

7 participants