Posted by Shyamala on January 16, 2013 at 3:36pm
17 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | javascript |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs work |
| Issue tags: | d8mux, d8mux-admin, mobile, mobile-novice, Needs manual testing, Responsive Design, Usability |
Issue Summary
Meta Issue:#1870944: [Meta] Mobile friendly admin pages
Problem/Motivation
Alignment of text in action dropdown. In Narrow screens the text is partly hidden.
Admin URL:
admin/config/content/formats (text: configure)
admin/structure/menu (text: list links)
drupal/admin/structure/taxonomy
Proposed resolution
needs new css approach (see @nod_ #47 on js performance)
Remaining tasks
- (on hold for new approach) Manual testing (see contributor task doc for manual testing: http://drupal.org/node/1489010)
- needs new approach (see @nod_ #47 on js performance) Create patch (and interdiff) (see contributor task doc for creating patch: http://drupal.org/node/1424598)
- (on hold for new approach) update issue summary with accurate proposed resolution and screenshots before and after
Comments
#1
Novice tags
#2
Any thoughts as to what would be a good solution.
I'd be happy to take the task on but not sure of the best way to move forward
#3
Attached patch does not use position:absolute in mobile. I tested also other places where the dropwidget is used, like admin/structure/menu
#4
Something which really helps to review such issues are two screenshots showing before and after the patch.
#5
1. Before screenshot of url /admin/config/content/formats
2. After screenshot of url /admin/config/content/formats
#6
#7
Note that #3 patch, removing position: absolute; stretches the buttons wide, and the plain button that is not a drop-button is equally as wide. Screenshots attached.
#8
Did a bit more testing. In addition to the point raised in #7, the patch from #3 causes a change in the centring of the text in other columns when the drop down is expanded.
1. Before screenshot of url /admin/config/content/formats - drop down expanded
2. After screenshot of url /admin/config/content/formats - drop down expanded
#9
Ignore #9 - finger trouble
#10
Another approach would be to use javascript to set the minimum width of the dropbutton-wrapper to that of the widest dropbutton. This patch attempts to implement this approach.
#11
about the issues in #7, you are right, but those would be solved if the media query of patch #3 is set to 400 instead of 901. I need to measure more exactly at which width the issue starts and use that value in the media query. I cannot make a patch at the moment. On the other hand the configure text might have a different length in other languages so the js approach of the latest patch is then safer, I will test when I find wifi.
#12
@10 patch, awesome! I hope you're on to the solution, but I found that when I click disable and cancel, or just click configure of one of the drop buttons, then go back to the previous screen (add text format) the js fix is no longer being used, the buttons are squished again, until I clear the site's cache, reload doesn't do it.
#13
#14
#15
On further investigation to get the correct button width the button must be positioned relative. Additionally, before calculating the width the button wrapper must be floated so it doesn't take the column width when the screen is wide.
This version of the patch takes the following approach:
1. Initially set the button wrapper and widget to floated left and position relative, respectively.
2. Use js to calculate the widest button widget.
3. Set the width of all the button widgets to the widest one.
4. Use js to reset the button wrapper and widget styles so they display correctly.
Would be good to get feedback on whether this is a valid approach.
#16
#17
Update to the comments to better fit in with drupal standards.
#18
#17: dropbutton-text-width-1890266-17.patch queued for re-testing.
#19
The last submitted patch, dropbutton-text-width-1890266-17.patch, failed testing.
#20
Re-rolled patch in case that's why it's failing
#21
#22
Thanks for your work on this everyone, especially @edrupal. Coding standards/documentation review follows:
+++ b/core/misc/dropbutton/dropbutton.jsundefined@@ -161,4 +175,24 @@ $.extend(Drupal.theme, {
+* Returns a string representing the wider of button or $widestButton.
Can this be reworded? "the wider of button or $widestButton" sounds odd. Maybe "Compares button and $widestButton and returns the largest value."? See also point #6.
+++ b/core/misc/dropbutton/dropbutton.jsundefined@@ -161,4 +175,24 @@ $.extend(Drupal.theme, {
+* A jQuery buttom element
s/buttom/button. These comments also need to end in a period per http://drupal.org/node/1354#drupal.
+++ b/core/misc/dropbutton/dropbutton.jsundefined@@ -161,4 +175,24 @@ $.extend(Drupal.theme, {
+* @param {String} $widestButton
+* A string representing the width to compare against the button element
String is redundant, how about removing "A string representing”, so it’s “The width to compare…”?
+++ b/core/misc/dropbutton/dropbutton.jsundefined@@ -161,4 +175,24 @@ $.extend(Drupal.theme, {
+* @return {String}
+* A string representing the wider of button and $widestButton
Again, string is redundant – maybe just “The largest width value."?
+++ b/core/misc/dropbutton/dropbutton.jsundefined@@ -161,4 +175,24 @@ $.extend(Drupal.theme, {
+ var buttonWidth = $('.dropbutton-widget', $button).outerWidth();
+ if (buttonWidth > $widestButton) {
+ return buttonWidth;
+ } else return $widestButton;
These lines need to be indented by two spaces instead of one.
+++ b/core/misc/dropbutton/dropbutton.jsundefined@@ -161,4 +175,24 @@ $.extend(Drupal.theme, {
+* @param {Jquery} button
...
+* @param {String} $widestButton
Is there a reason why the second parameter has a $ and the first doesn't?
#23
Thanks for the excellent feedback @Cottser. I've incorporated all the comments into this latest patch.
#24
+++ b/core/misc/dropbutton/dropbutton.base.cssundefined
@@ -17,6 +17,10 @@
+.js .dropbutton-wrapper.no-float {
@@ -37,7 +41,7 @@
+.js .dropbutton-widget.position-absolute {
Why are we using class names that map 1:1 with CSS properties? I don't know of anywhere else in core that is done, or why it is appropriate here.
#25
Thanks for the quick turnaround, @edrupal! Agreed with @tim.plunkett, those class names could be improved, and maybe combined into one class name - something like "processed"? Not sure what the convention is there.
Some more documentation/standards suggestions and notes:
+++ b/core/misc/dropbutton/dropbutton.jsundefined@@ -14,10 +14,24 @@ Drupal.behaviors.dropButton = {
+ /**
+ * Set minimum width of first drop buttom wrapper to that of the widest
+ * dropButton to prevent button text overflow when screen width is small.
+ */
Wording on this could be improved (parts are a bit terse), and this should be an inline comment (//). Something like "Set the minimum width of the first dropbutton wrapper to that of the widest dropbutton to prevent button text overflow on small screens."
+++ b/core/misc/dropbutton/dropbutton.jsundefined@@ -14,10 +14,24 @@ Drupal.behaviors.dropButton = {
+ /**
+ * In order to accurately calculate the button width, the button wrapper
+ * had to be set to float and widget set to position relative.
+ * These settings must be undone to ensure the button displays correctly.
+ */
This should be an inline comment as well. This comment confused me at first, here's another suggestion but could be improved by someone who understands this patch more than I do: "In order to accurately calculate the button width, the button wrapper is initially floated and the widget is positioned relatively."
On the next line, can we replace "settings must be undone" with "attributes must be overridden" or something along those lines? CSS doesn't have settings :)
+++ b/core/misc/dropbutton/dropbutton.jsundefined@@ -176,23 +176,25 @@ $.extend(Drupal.theme, {
+* Compares the width of $button to the width of $widestButton and returns the
+* the larger value.
This is now too long, the summary line must be under 80 characters per http://drupal.org/node/1354#drupal.
+++ b/core/misc/dropbutton/dropbutton.jsundefined@@ -176,23 +176,25 @@ $.extend(Drupal.theme, {
-* @param {String} $widestButton
...
+* @param $widestButton
...
-* @return {String}
...
+* @return
Sorry for the confusion - I meant the "A string…" text from the parameter description was redundant. The {String} data types should be kept.
#26
+++ b/core/misc/dropbutton/dropbutton.jsundefined@@ -161,4 +175,26 @@ $.extend(Drupal.theme, {
+* A jQuery $buttom element.
Oops, this was missed. Should be "$button" :)
#27
Thanks for your patience and feedback. Here's another version.
#28
Typo correction and change of name of the 'processed' class.
#29
Tagging.
#30
Thanks @edrupal, looks good to me. Only a couple more tiny nits to pick, leaving at needs review though:
+++ b/core/misc/dropbutton/dropbutton.jsundefined@@ -161,4 +170,25 @@ $.extend(Drupal.theme, {
+* @return {String}
+* The largest width value.
+*
+*/
Extra line here after the @return description and before the
*/is not needed.+++ b/core/misc/dropbutton/dropbutton.jsundefined@@ -20,18 +20,13 @@ Drupal.behaviors.dropButton = {
+ // Add a class to the dropbutton-wrapper and dropbutton-widget html
html should be capitalized.
Edit: Added more context to @return code snippet.
#31
updated issue summary remaining tasks section.
#32
Thanks. Updated patch and another slight change to name of 'processed' class.
#33
these changes to the code look good.
I noticed some of the lines could have been wrapper longer but still under 80 chars.
Also, there was a block of comments missing one space indentation.
I made those changes.
next steps here: manual testing (see contributor task doc on manual testing: http://drupal.org/node/1489010)
#34
at first this looked really good.
before patch: cuts off
after patch:
no cut off
then I noticed that the row height changed when the button expanded making the other rows jump and move.
this does not happen without the patch.
I was looking for a button that had a short word on top, and a longer word under.
with translation enabled, the content list has that.
I noticed that the column widths change when the button is expanded.
this does not happen without the patch.
compare this:
with this:
with no patch:
with no patch: goes over edge.
I tried to apply the patch and get the row height to change again...
now I cannot reproduce the row height changing and I'm confused. I cleared caches on the site, and the browser cache... I'm not sure why.
Posting what I have now. Me or someone else can come back and do another round of manual testing.
Q1: Do we want the row height to change when the drop button is expanded? (I think we do not want that.)
Q2: Is column width changing when the drop button is expanded a blocker to this? (I think yes.)
In general, I think we dont want the page to jump around.
The approach in the patch is to set the width of the first item in the button to be the width of the widest button.
#35
The @params need some touch-up, see these comments on another issue (#77, #81, #83, #85):
#1847084-77: Measure/track displacing elements better + provide change events for them (fix overlay + toolbar)
#1847084-81: Measure/track displacing elements better + provide change events for them (fix overlay + toolbar)
#1847084-83: Measure/track displacing elements better + provide change events for them (fix overlay + toolbar)
#1847084-85: Measure/track displacing elements better + provide change events for them (fix overlay + toolbar)
In short: Remove the curly braces, remove the blank lines between each @param, and I would think Jquery needs to be jQuery.
#36
Hi @Cottser, would it be best practice to change the @param blocks in all the other functions in the program dropbutton.js so there is consistency, even if they are not related to this issue?
#37
@edrupal - Good question. We should only change the ones in this patch, and we'll file followup issue(s) to correct the existing curly braces and extra lines.
#38
Please see #1958246-2: Update JS documentation to follow Doxygen format (remove curly braces) - in addition to #35 it looks like button should be $button and widestButton should be widestButton. JavaScript docs are complicated! :)
#39
As well as the issues already identified in #34 the previous patch also caused problems on the views admin pages where the hidden buttons in the advanced section where not sized correctly.
I've re-written the patch. The principle has not changed - to calculate the width of the widest text in the button and set the button width to this value.
Now the change is restricted to only work on buttons that are in the column of a table, and makes sure there are no hidden button elements before calculating its width.
This change is to ensure buttons that are not part of a column table are unaffected (which is where all the known issues occur).
1a. Content list - before patch applied (/admin/content)
1b. Content list - after patch applied (/admin/content)
2a. Text format configuration - button closed - before patch applied (/admin/config/content/formats)
2b. Text format configuration - button closed - after patch applied (/admin/config/content/formats)
3a. Text format configuration - button open - before patch applied (/admin/config/content/formats)
3b. Text format configuration - button open - after patch applied (/admin/config/content/formats)
#40
The last submitted patch, dropbutton-text-width-1890266-39.patch, failed testing.
#41
#39: dropbutton-text-width-1890266-39.patch queued for re-testing.
#42
Re @YesCT:
I tend to agree with you but these two details were specifically implemented the way they are in the original implementation. So if that bugs you I think you have to open a new issue and fight Bojhan or yoroy :-)
#43
#42, the way the dropbuttons currently work does not expand row height or width when expanded, because of the absolute positioning.
#44
I like the functionality of this patch a lot.
No jumping around of the table rows. :)
Removing needs manual testing tag. (I did not check views, but looks like @edrupal did)
I'm not sure about the need for $ or no $ as @Cottser mentioned in #38, so a standards check of js would be nice.
#45
Minor update to correct the variable names to fit in with standards.
#46
+++ b/core/misc/dropbutton/dropbutton.jsundefined@@ -17,6 +17,17 @@ Drupal.behaviors.dropButton = {
+ if ($($dropbuttons[i]).parents('td').length > 0) {
...
+ var buttonOuterWidth = $($dropbuttons[i]).width();
...
+ $($dropbuttons[i]).width(buttonOuterWidth);
...
+ $($dropbuttons[i]).addClass('width-processed');
These should also probably be assigned into a variable first
+++ b/core/misc/dropbutton/dropbutton.jsundefined@@ -17,6 +17,17 @@ Drupal.behaviors.dropButton = {
+ var buttonWidth = $('.dropbutton-widget', $dropbuttons[i]).width();
...
+ $('.dropbutton-widget', $dropbuttons[i]).width(buttonWidth);
In both places here we do
$('.dropbutton-widget', $dropbuttons[i]);, why not assign that to a variable first?#47
I'm really not comfortable with the JS that's added here.
Before the patch, 1 dropbutton took 1ms to initialize. After the patch: 5ms. After the patch and some work on it: 4ms.
There have been lots of work to make the footprint of dropbutton JS as low as possible, this totally defeats the purpose. Think of a view with 50 dropbuttons on a mobile phone (like the admin/content page for example, it'll be a view at some point). Rendering takes 50ms. With the patch it takes 250ms. On a mobile? it takes 500ms and with the patch it would take 2.5 seconds, on top of all jQuery, Backbone and whatever else is on the page. This can not work.
Also we can't be using the -processed class in our CSS. It comes from the jQuery.once plugin and that plugin will soon be using data-attributes that you shouldn't be using in CSS to begin with.
I'm not sure of the way forward but it's not a JS-heavy one. Try to see if a CSS-only solution will work out please. you'll probably have to mess around with the current styling.
I can't support a patch that will make admin/content page take 2.5s to render mobile device because of dropbuttons.
#48
If you're concerned about the js then another solution would be just set the min-width to 100px, I don't know you guys feel about that approach.
#49
@nod_ 's comment on performance is pretty important.
related:
#1849750: minimize all "Operations" columns width
#1914800: Dropbutton width is smaller than longest item
updating issue summary.