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] Improving row selection in Joomla! Admin #14913

Merged
merged 8 commits into from Apr 1, 2017

Conversation

jreys
Copy link
Contributor

@jreys jreys commented Mar 26, 2017

Summary of Changes

Usually when I was using Joomla Admin, the multi-select on the list view was not very user-friendly since clicking on such a small square can sometimes fail on the first try. So I came up with a solution for

Testing Instructions

  • Go to articles (or any multi-select list view), try clicking anywhere on a row, it will select the whole row
  • Try trashing multiple items by clicking severall rows
  • Try the toggle-all button

Expected result

When you click on a row on the articles list view (for example), it will highlight the whole line:
image

When you click on the select-all checkbox, it will highlight all items on that page:
image

Notice

I'm aware of backwards compatibility so the regular checkbox will still work as before while still highlighting the whole line.

@jreys jreys changed the title Improving row selection in Joomla! Admin [4.0] Improving row selection in Joomla! Admin Mar 26, 2017
@dgrammatiko
Copy link
Contributor

Can you drop jquery (you don't need it for such easy task) and use a class instead of changing directly the styles

@jreys
Copy link
Contributor Author

jreys commented Mar 26, 2017

and use a class instead of changing directly the styles

Do you mean adding a class to the row when the action is done?

@C-Lodder
Copy link
Member

C-Lodder commented Mar 26, 2017

  • Chain variables
  • No jQuery please
  • Define your colour in the Atum variables.scss
  • Add a class, apply background, call the colour
  • Compile SCSS
  • Use JS (element.classList.toggle('foo');) to toggle the class

@jreys
Copy link
Contributor Author

jreys commented Mar 26, 2017

  • Chain variables
  • No jQuery please
  • Define your colour in the Atum variables.scss
  • Add a class, apply background, call the colour
  • Compile SCSS
  • Use JS (element.classList.toggle('foo');) to toggle the class

I have questions on the topics not checked:

  • What do you understand as Chain variables?
  • how do I compile SCSS correctly?
  • the last one seems obvious after SCSS is compiled tbh

@dgrammatiko
Copy link
Contributor

"use strict";
var color = '#d9edf7', rows   = document.querySelectorAll('tr[class^="row"]');

The above snippet should take care of the first checkbox.

For compiling SCSS you need to have installed node and npm
Then in the root directory of joomla run npm install
Once complete run grunt compile

@C-Lodder
Copy link
Member

C-Lodder commented Mar 27, 2017

Here are the instructions for SCSS: https://github.com/joomla/joomla-cms/blob/4.0-dev/grunt-readme.md

@@ -54,6 +54,7 @@ $fa-font-path: "../../../../media/vendor/font-awesome/fonts"
// Tables
$table-bg: transparent !default;
$table-bg-accent: rgba(0,0,0,.03) !default;
$table-row-selected #d9edf7;
Copy link
Member

Choose a reason for hiding this comment

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

add a colon after the variable name, like so: $table-row-selected:


document.addEventListener("DOMContentLoaded", function(event) {
color = '#d9edf7';
rows = document.querySelectorAll('tr[class^="row"]');
Copy link
Member

@C-Lodder C-Lodder Mar 27, 2017

Choose a reason for hiding this comment

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

change to:

var colour = '#d9edf7',
    rows   = document.querySelectorAll('tr[class^="row"]');

Note, the British spelling of colour

rows.forEach(function(row, index) {
changeBg(row, '');
});
}
Copy link
Member

@C-Lodder C-Lodder Mar 27, 2017

Choose a reason for hiding this comment

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

you can simplify this if - else statement to just:

var colour = this.checked ? colour : '';

rows.forEach(function(row, index) {
	changeBg(row, colour);
});

@jreys
Copy link
Contributor Author

jreys commented Mar 27, 2017

I think that is all :)

@ghost
Copy link

ghost commented Mar 27, 2017

@jreys is this PR ready to Test?

@C-Lodder
Copy link
Member

@jreys - one more thing, please use single quote's as opposed to double quotes where possible.

@jreys
Copy link
Contributor Author

jreys commented Mar 27, 2017

@franz-wohlkoenig it's ready for testing now, everything fixed

btw @C-Lodder I didn't use the element.classList.toggle('foo'); because it was causing weird behaviours when you selected one row and then selected them all after, it would leave the previous row unselected and it's not the wanted behaviour

@C-Lodder
Copy link
Member

@jreys - No worries. All seems to be good now.

@dgt41 - JS look ok?

@C-Lodder
Copy link
Member

I have tested this item ✅ successfully on 0a5a387


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

@ghost
Copy link

ghost commented Mar 27, 2017

I have tested this item ✅ successfully on 0a5a387

an optical Feedback like another Curser would help to see that i can move Column.


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

@jeckodevelopment
Copy link
Member

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Mar 27, 2017
@jeckodevelopment jeckodevelopment added this to the Joomla 4.0 milestone Mar 27, 2017
@cokencorn
Copy link

I have tested this item ✅ successfully on ac8c87d


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

@wilsonge wilsonge merged commit 8139b08 into joomla:4.0-dev Apr 1, 2017
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Apr 1, 2017
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.

None yet

7 participants