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

Difference in height of visibility button and password input #33823 #33824

Closed
wants to merge 1 commit into from
Closed

Difference in height of visibility button and password input #33823 #33824

wants to merge 1 commit into from

Conversation

deveshprasad
Copy link
Contributor

Pull Request for Issue #33823

Summary of Changes

Difference in the height of input and visibility button
Solved by adding a bootstrap bottom margin to 0.

Testing Instructions

Go to the main joomla web page and view it on mobile view.

<button type="button" class="btn btn-primary js-pstats-btn-allow-always m-1"><?php echo Text::_('PLG_SYSTEM_STATS_BTN_SEND_ALWAYS'); ?></button> <button type="button" class="btn btn-primary js-pstats-btn-allow-once m-1"><?php echo Text::_('PLG_SYSTEM_STATS_BTN_SEND_NOW'); ?></button> <button type="button" class="btn btn-primary js-pstats-btn-allow-never m-1" style="margin:0px"><?php echo Text::_('PLG_SYSTEM_STATS_BTN_NEVER_SEND'); ?></button>

Actual result BEFORE applying this Pull Request

10 05 2021_20 55 28_REC

Expected result AFTER applying this Pull Request

11 05 2021_21 10 38_REC

Documentation Changes Required

No changes in the documentation.

@rjharishabh
Copy link
Contributor

I have tested this item 🔴 unsuccessfully on 48a2894


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

@rjharishabh
Copy link
Contributor

It's the same as before
erro

And please change your branch to something other, not 4.0-dev

Copy link
Contributor

@rjharishabh rjharishabh left a comment

Choose a reason for hiding this comment

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

Please change bottom-margin to 0
See below comment

@brianteeman
Copy link
Contributor

The problem is caused by

@media (max-width: 767.98px)
.btn {
    margin-bottom: .25rem;
}

@rjharishabh
Copy link
Contributor

rjharishabh commented May 12, 2021

Sorry, it's by mistake approved and I didn't revert it.
So, please don't consider these changes approved

@deveshprasad
Copy link
Contributor Author

I followed the GSOC guidelines to change the branch to 4.0-dev, so which branch should I choose?

@richard67
Copy link
Member

I followed the GSOC guidelines to change the branch to 4.0-dev, so which branch should I choose?

@deveshprasad The branch is ok. It would have been better if you had created a new, extra branch for this PR based on the 4.0-dev branch, but as long as you don't make other PR's for other issues, it works like now, using your 4.0-dev branch.

Later when this PR is ok and tested and merged, the best is you delete your 4.0-dev branch and create it again based on the 4.0-dev branch of the CMS. Otherwise, if you don't do that and later create another PR based on your 4.0-dev branch which still contains your changes from this PR here, the other, future PR will be wrong.

But for now that should not be a problem.

@rjharishabh
Copy link
Contributor

Here you should change margin-bottom to 0

@brianteeman
Copy link
Contributor

Here you should change margin-bottom to 0

  1. If the intent is to remove the margin-bottom at that size then you should just remove the entire class not set it to 0
  2. Whenever you remove something you should see why it was there in the first place. Everything has a purpose so you wouldnt want to break that with a change

The PR that introduced that code was #32697

@brianteeman
Copy link
Contributor

I have tested this item 🔴 unsuccessfully on 48a2894

Breaks all buttons see comments


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

@rjharishabh
Copy link
Contributor

Then it can be corrected by add mb-0 to it's markup.
That's in password. php file.

@brianteeman
Copy link
Contributor

That would set a margin bottom at all screen sizes. Is that what you want to do?

@deveshprasad
Copy link
Contributor Author

So if the bootstrap margin is not the correct way and we should remove the entire class, and should directly set it to 0.
I tried applying bootstrap margin individually ( .btn) so that it won't affect the other buttons. But this is still not the correct way,
so can we add a new class on this button and which has its margin in the CSS files equal to 0?

I have a similar issue which I solved with help of bootstrap, but if m-1 is not the correct way as it works on all responsive screens
then can I add a new class because removing the current one will remove all those styling related with that class.

Similar issue:

1

@rjharishabh
Copy link
Contributor

Same issue at the change password page in the frontend.
change-pass

If we add mb-0 to this line then both problem will be solved, It's not affecting other screen sizes

<button type="button" class="btn btn-secondary input-password-toggle">

@deveshprasad
Copy link
Contributor Author

13 05 2021_15 45 42_REC
13 05 2021_20 50 17_REC

Similar issue on mobile view .
If we want to target one particular screen without affecting other screen size, and by using bootstrap we can use
mb-md-0 for the previous issue and mt-md-1 for this issue ?

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.

5 participants