Skip to content

Conversation

@demis-palma
Copy link
Contributor

@demis-palma demis-palma commented Mar 28, 2017

Chosen dropdown provides 2 ways to set the width of the dropdown element:

  1. Specify it as inline style in the select element (<select style="width: 500px;" ...) or
  2. Specify it as an option for the chosen() function (width: '600px')
    See related documentation.

The new style introduced in the Isis template forces the width to a different value (which also causes a horizontal collapse in case of blank items).

There is no reasonable way for third party extensions developers to specify a custom with for their own dropdown elements.

To fix this problem, I've removed two css lines that in principle, we should not use anyway, since the chosen dropdown control offers other ways to set its width.

However it's possible that @ciar4n remembers the reason for these lines and can help us by providing further details.

Testing Instructions

Create a select anywhere in the back-end

<?php
JFactory::getDocument()->addStyleSheet(JUri::root(true) . "/media/jui/css/chosen.css");
JFactory::getDocument()->addScript(JUri::root(true) . "/media/jui/js/chosen.jquery.js");

<div class="control-group">
	<select style="width: 500px;" class="chosenize">
		<option>Option</option>
	</select>
</div>

Pay attention to encase it in a "control-group" wrapper as shown, since the issue only affects dropdown elements within a "control-group" wrapper.
Also pay attention to the class assigned (in this case "chosenize"), which must correspond to that used in the following JavaScript.

Run the following JavaScript to transform the select into a chosen dropdown:

jQuery(document).ready(function ($)
{
	// 600px overrides the value of 500px specified in the PHP code
	$('.chosenize').chosen({ width: '600px' });
});

Expected result

I expect the dropdown was 500 or 600 px wide, with the precedence to the value specified in the JavaScript.

Actual result

The dropdown is 220 px wide.

@ciar4n
Copy link
Contributor

ciar4n commented Mar 28, 2017

Thank you @demis-palma

The reason for this CSS was to make the chosen fields fluid/responsive #13586 .

You should still be able to set your field width by using max-width instead of width ?

@demis-palma
Copy link
Contributor Author

OK @ciar4n congratulations for your work, it looks like very nice.
However, for some reason, it does not work as expected.
The screenshot is taken in the Global configuration, and it does not seems always responsive.
I am currently trying to find the cause.

screenshot1

@demis-palma
Copy link
Contributor Author

I've found the reason. Tomorrow I'll commit a possible fix.

@joomla-cms-bot joomla-cms-bot changed the title New Isis tempalte prevents chosen dropdown from working properly New Isis template prevents chosen dropdown from working properly Mar 30, 2017
@demis-palma
Copy link
Contributor Author

I've found that dropdown are responsive and can shrink if it contains a short text. On the contrary, if it contains a longer text, its width it's always 220px. No more, no less.

I'm working on a fix.

screenshot3

@ciar4n
Copy link
Contributor

ciar4n commented Mar 30, 2017

Admittedly I am unable to replicate the issue you are presenting. Your screenshot appears to be indicating a screen width narrower than any device I am aware of. Note that the smallest width that should be currently considered is no less than 320px.

@demis-palma
Copy link
Contributor Author

"I am unable to replicate the issue you are presenting."

Just resize your browser window under 200px width.

"the smallest width that should be currently considered is no less than 320px."

Where did you read that? It sounds absurd to me.

To resumen, this PR solves two different problems:

  1. I got everything responsive above and below 320px.
  2. I got third party chosen dropdown working and still responsive

screenshot4

@brianteeman
Copy link
Contributor

brianteeman commented Mar 30, 2017 via email

@demis-palma
Copy link
Contributor Author

a lot of things that break at that size

OK, but make it non working intentionally, is evil. :)

@demis-palma
Copy link
Contributor Author

OK, forget about the responsiveness, which is only a plus.
Do you confirm that this PR solves the main issue explained above?

@ciar4n
Copy link
Contributor

ciar4n commented Mar 31, 2017

A 320px min screen width is justified as little or no devices exist below this... http://viewportsizes.com/

Based on code review it appears this will revert #13586. Regarding the issue this PR originally addressed, as previously stated you should still be able to set your field width by using max-width instead of width.

@demis-palma
Copy link
Contributor Author

Based on code review it appears this will revert #13586.

Since you are the main author of this Isis restyling, you should really take the time to test this PR.
I've tested it extensively, and it does not revert #13586. Chosen fields and the yes/no buttons are still responsive/fluid. @ciar4n please confirm this.

Regarding the issue this PR originally addressed, as previously stated you should still be able to set your field width by using max-width instead of width.

I should be able to do whatever I want, but the problem it's not me. I've pointed out that the problem affects extensions developers. You won't believe, but they usually expect that things work as the related documentation explains.

With this PR we have the opportunity to

  1. make things work as documented,
  2. improving the responsiveness on small displays,
  3. simplifying the code removing those "!important" statements that are a pain in the rear for third party extensions to override,
  4. all this without any known drawback.

So, what we are waiting for?

@zero-24
Copy link
Contributor

zero-24 commented Apr 8, 2017

Chosen fields and the yes/no buttons are still responsive/fluid. @ciar4n please confirm this.

Old code:
Starting at: image

After rezising:
image

new code
starting at:
image

After rezising:
image

I guess this is what @ciar4n is refering to.

btw i dont understand the example js code too. as nothing happens after that even with the patch:
image

Without the patch we just can't add a dropdown with more than 220px.

But i can't help here much as i'm not a JS nor CSS Pro.

@ciar4n
Copy link
Contributor

ciar4n commented Apr 8, 2017

I've tested and this PR does revert #13586 ...

37-fields

@demis-palma brings up a fair point regarding setting an alternative width to dropdowns which may cause issues for 3PD.

#13586 could be just applied to the edit screens sidebar as this is the only real area where responsive fields is required in the Joomla views. Alternatively we can simply revert #13586 as it's need is not huge (minimal screen sizes effected).

For this PR to correctly revert #13586, @demis-palma please undo 16a3702. Two tests have shown this commit to be incorrect.

@demis-palma
Copy link
Contributor Author

@ciar4n thanks for getting back to this.
I think that I’ve found the reason why on your tests the current PR reverts PR #13586 and I’ve fixed the problem with the latest commit. You are certainly working with Firefox, and you are victim of its non-responsive fieldsets described here

I've also just realised that we were talking about two different screens. I was talking about Global Configuration, while you were talking about Article Manager.
The reason why we don’t care about areas below 320px in Global Configuration screen, while we care about them in Article Manager screen, goes beyond my comprehension. However, I'm okay with talking about Article Manager.

I’m pretty sure that with the latest commit you’ll confirm this PR. Anyway, I’ve prepared these screenshots.

The first one corresponds to the current Isis behaviour.
Here we are in Article Manager, in a 200px wide area. The chosen container assumes a width of 145px due to your css rules highlighted.

screenshot1

The second instead, corresponds to the current PR.
Again in Article Manager, in a 200px wide area. The chosen container still assumes a width of 145px due to different css rules highlighted.

screenshot2

@ciar4n
Copy link
Contributor

ciar4n commented Apr 10, 2017

@demis-palma Good work! The PR now works as described and is a much better solution than #13586 as it should resolve any B/C break.

This PR now reverts #13586 and provides a better alternative solution.

The reason why we don’t care about areas below 320px in Global Configuration screen, while we care about them in Article Manager screen, goes beyond my comprehension.

The reason here is due to the article editor displaying in two columns so responsive fields are required at a higher screen width.

@ciar4n
Copy link
Contributor

ciar4n commented Apr 10, 2017

I have tested this item ✅ successfully on 0627453


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

@demis-palma
Copy link
Contributor Author

Great, thank you!

@ghost
Copy link

ghost commented Apr 11, 2017

@demis-palma can you please write what Tester have to look for?

@ciar4n
Copy link
Contributor

ciar4n commented Apr 11, 2017

@franz-wohlkoenig To test you can check that #13529 is still resolved

@ghost
Copy link

ghost commented Apr 11, 2017

I have tested this item ✅ successfully on 0627453


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

@ghost
Copy link

ghost commented Apr 11, 2017

RTC after two successful tests.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Apr 11, 2017
@demis-palma
Copy link
Contributor Author

The tests consists basically in verifying that the PR respects the element width specified as described at the top of this page, but also the chosen element continues being fluid / responsive.

Note that we have tested Article Manager and Global configuration screens only, so that testing it in a different screen would be even better.

@joomla-cms-bot joomla-cms-bot added this to the Joomla 3.7.0 milestone Apr 11, 2017
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Apr 11, 2017
@joomla-cms-bot joomla-cms-bot modified the milestone: Joomla 3.7.0 Apr 11, 2017
@infograf768
Copy link
Member

RTC again + milestone...


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Apr 11, 2017
@zero-24 zero-24 added this to the Joomla 3.7.0 milestone Apr 11, 2017
@rdeutz rdeutz merged commit 2b6cb21 into joomla:staging Apr 11, 2017
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Apr 11, 2017
@demis-palma demis-palma deleted the chosen-width branch May 4, 2017 13:15
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