NPM Resource Changed ? Pending

User tests: Successful: Unsuccessful:

avatar manakwadhwa2002
manakwadhwa2002
23 Mar 2021

Fixed Action toggle icon covers search go button

Pull Request for Issue #32794

Summary of Changes

Added margin-right to .input-group

Testing Instructions

Can test on any mobile device

Actual result BEFORE applying this Pull Request

Before PR

Expected result AFTER applying this Pull Request

After PR

Documentation Changes Required

None

avatar manakwadhwa2002 manakwadhwa2002 - open - 23 Mar 2021
avatar manakwadhwa2002 manakwadhwa2002 - change - 23 Mar 2021
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 23 Mar 2021
Category Administration Templates (admin) NPM Change
avatar manakwadhwa2002 manakwadhwa2002 - change - 23 Mar 2021
Labels Added: NPM Resource Changed ?
avatar brianteeman
brianteeman - comment - 23 Mar 2021

Please check with an RTL language

avatar manakwadhwa2002
manakwadhwa2002 - comment - 23 Mar 2021

@brianteeman sir now you can test the code so that you can merge it if passes.

avatar PhilETaylor
PhilETaylor - comment - 23 Mar 2021

Alternative approach that conflicts with this PR #32818

avatar PhilETaylor
PhilETaylor - comment - 23 Mar 2021

Another alternative approach that conflicts with this PR is #32810

avatar manakwadhwa2002
manakwadhwa2002 - comment - 23 Mar 2021

@PhilETaylor I have also tested PR #32810. But I got the issue that the cog icon is covering the text, there is no chance that we need to shift this up, we are already facing the issue that this icon covers the text on header and if we will shift this up it will create an another issue. So instead of touching the cog icon I just shifted the search icon group as it will not affect other pages but if we shift cog icon it will have its affect on all other pages.

avatar PhilETaylor
PhilETaylor - comment - 23 Mar 2021

Its not my decision on which PR the project accepts, I was just bringing it to your, and the maintainers attention that there are multiple, and conflicting, approaches to this issue currently.

avatar manakwadhwa2002
manakwadhwa2002 - comment - 23 Mar 2021

@PhilETaylor sure I understand that.

avatar infograf768
infograf768 - comment - 23 Mar 2021

This does not work at all in ltr or rtl as the new classes are a sub of .toggler-toolbar
If we separate the new classes to something like:

@include media-breakpoint-down(sm) {
  .toggler-toolbar {
    top: 20px;
    z-index: $zindex-alerts;

    [dir=ltr] & {
      right: 10px;
    }

    [dir=rtl] & {
      left: 10px;
    }

    .toggler-toolbar-icon::before {
      font: normal normal 900 30px/1 "Font Awesome 5 Free";
      color: var(--toggle-color);
      content: "\f00d";
    }

    &.collapsed .toggler-toolbar-icon::before {
      content: "\f085";
    }
  }

  .input-group {
    margin-right: 70px;
 
    [dir=rtl] & {
      margin-right: auto;
      margin-left: 70px;
    }
  }

  .header {
    margin-bottom: 10px;
  }

  .subhead {
  [...]

Then it works in both ltr and rtl but the results can be weird on some managers.

Screen Shot 2021-03-23 at 11 13 27

avatar manakwadhwa2002
manakwadhwa2002 - comment - 23 Mar 2021

@infograf768 Ok, So should I change it as you have recommended ?

avatar richard67
richard67 - comment - 23 Mar 2021

@manakwadhwa2002 Maybe close your PR in favour of #32810 and focus on another issue?

avatar infograf768
infograf768 - comment - 23 Mar 2021

Honestly, I don't know which is the best solution between this PR and
#32818
#32810

I myself prefer 32810

avatar drmenzelit
drmenzelit - comment - 23 Mar 2021

As @infograf768 stated before, this PR is not working. @manakwadhwa2002 thank you very much for your contribution, we are closing this PR in favour of #32810

avatar drmenzelit drmenzelit - change - 23 Mar 2021
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2021-03-23 10:46:51
Closed_By drmenzelit
avatar drmenzelit drmenzelit - close - 23 Mar 2021

Add a Comment

Login with GitHub to post a comment