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] Center modal on mobile #27859

Merged
merged 1 commit into from
Feb 9, 2020
Merged

Conversation

brianteeman
Copy link
Contributor

@brianteeman brianteeman commented Feb 8, 2020

Pull Request for Issue #27536 .

Summary of Changes

Bizarrely bootstrap does not center modals on devices smaller than 567px eg mobile phones

Testing Instructions

Go to the admin dashboard
Reduce viewport width down to a mobile size
Click the "Add module to the dashboard" button at the bottom of the page

Expected result

Horizontally aligned modal

Actual result

Modal aligned to the left

apply the pr run npm i or node build.js --compile-css

@richard67
Copy link
Member

I have tested this item ✅ successfully on 71b426d


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

1 similar comment
@Quy
Copy link
Contributor

Quy commented Feb 9, 2020

I have tested this item ✅ successfully on 71b426d


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

@Quy Quy removed the PR-4.0-dev label Feb 9, 2020
@Quy
Copy link
Contributor

Quy commented Feb 9, 2020

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Feb 9, 2020
@richard67 richard67 merged commit 9ed75bf into joomla:4.0-dev Feb 9, 2020
@joomla-cms-bot joomla-cms-bot added PR-4.0-dev and removed RTC This Pull Request is Ready To Commit labels Feb 9, 2020
@richard67 richard67 added this to the Joomla 4.0 milestone Feb 9, 2020
@richard67
Copy link
Member

Thanks.

@brianteeman
Copy link
Contributor Author

thanks

@brianteeman brianteeman deleted the mobile_modal branch February 9, 2020 18:19
@chmst
Copy link
Contributor

chmst commented Feb 9, 2020

I don't agree with this change. I think changes should never be made in the vendor directory but in the overrides.


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

@brianteeman
Copy link
Contributor Author

@chmst as far as I understand that is the folder for vendor overrides

@richard67
Copy link
Member

@chmst as far as I understand that is the folder for vendor overrides

Was my understanding, too, but I might be wrong.

@Quy
Copy link
Contributor

Quy commented Feb 9, 2020

@chmst I think you are right. It should have been modified here:
\administrator\templates\atum\scss\blocks\_modals.scss

@richard67
Copy link
Member

@chmst I think you are right. It should have been modified here:
\administrator\templates\atum\scss\blocks\_modals.scss

@brianteeman Could you check and if necessary make a new PR?

@brianteeman
Copy link
Contributor Author

This PR was correct

@chmst
Copy link
Contributor

chmst commented Feb 9, 2020

Your changes are correct.
But the code would be easier maintain if you could do them in scss/blocks/_modals where all the overrides for modals are.


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

@brianteeman
Copy link
Contributor Author

That's the problem with us having two different places where code is being overridden. It would be easier to maintain if there was only one place and not two.

@infograf768
Copy link
Member

It is indeed very confusing where to modify/change stuff for this template.
The question is: are you willing to make a PR to move the code to \administrator\templates\atum\scss\blocks\_modals.scss or not.
If you are not willing to, no problem, I will do.

@brianteeman
Copy link
Contributor Author

If the code is moved then all the code in that vendor folder should be moved as well and the imports removed

@brianteeman
Copy link
Contributor Author

and/or it should be clearly documented when to use the vendor folder

@infograf768
Copy link
Member

Well, this case is quite clear:
atum vendor modal only deals with jviewport stuff

blocks/modal deals with all specific modal classes. btn, header, body, title, etc.
Therefore .modal-dialog should also be there.

@brianteeman
Copy link
Contributor Author

clear as mud. the logic behind the decision escapes me

@infograf768
Copy link
Member

The whole way Atum scss are made is indeed clear as mud. The use of svg is a p i t a (see #27864 )
Nevertheless and until someone modifies all of that, let's be consequent with what we have.
Will make PR.

@brianteeman
Copy link
Contributor Author

i dont see how you come to the conclusion that it is in the wrong place

@infograf768
Copy link
Member

I give up.

@N6REJ
Copy link
Contributor

N6REJ commented Feb 10, 2020

@brianteeman putting modal overrides in modals ( \administrator\templates\atum\scss\blocks\_modals.scss )
makes perfect sense to me. I get that it's confusing but it can be fixed one bite at a time. Lets at least get the ball rolling by putting your pr there.
I hope that makes sense to you.

@brianteeman
Copy link
Contributor Author

Just for clarity - it is not an override

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