There's no need to move the top item to the top or the bottom item to the bottom. Still, if the corresponding buttons are clicked, the item is being marked as touched. This doesn't happen with "move up" and "move down".

I'm very proud to post my first js patch. It's not more than adding an if-check, but I'm (unfortunately) absolutely no JS pro, so if this can be easier accomplished, go forward. But - at least - it works!

CommentFileSizeAuthor
#16 remove_top_bottom_buttons_16.patch6.37 KBPancho
#16 remove_top_bottom_buttons.jpg68.52 KBPancho
#8 remove_top_bottom_buttons.patch5.5 KBPancho
list.js_dont_touch.patch1.98 KBPancho
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Pancho’s picture

Pancho
Location UTC+2 🇪🇺 EU
CreditAttribution: Pancho commented
Title: Context table: no need to move upmost context to the top » Context table: no need to move upmost argument to the top

Three more suggestions:

- Instead of this we could show the buttons only if its action makes sense to the corresponding item. So for the first item we only show "down" and "bottom", for the last item only "up" and "top".

- Given that most configurations don't have more than two arguments, I'm not really sure we need "top" and "bottom" at all. This would mean: less cruft and less code.

- If we don't leave 'top' and 'bottom' out, we should at least rearrange the icons to be in the order: top - up - down - bottom. That makes logically more sense.

Pancho’s picture

Pancho
Location UTC+2 🇪🇺 EU
CreditAttribution: Pancho commented
Component: User interface » Attn -- m
merlinofchaos’s picture

merlinofchaos CreditAttribution: merlinofchaos commented

I'm ok with leaving top/bottom off, honestly. Even with 5 arguments it's not a big problem to move stuff around.

Pancho’s picture

Pancho
Location UTC+2 🇪🇺 EU
CreditAttribution: Pancho commented

Does this mean that I can also remove all JS code the top/bottom button functionality uses? That wouldn't be bad for performance...

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

+1 for removing top/bottom along with the JS code

Pancho’s picture

Pancho
Location UTC+2 🇪🇺 EU
CreditAttribution: Pancho commented

This patch should do it. Has undergone some testing, might need some more, though.

Top and bottom buttons are removed along with their JS code.
Also I'm completely removing the shuffle functionality. Don't believe we will ever need "shuffle" in Panels... :)

Pancho’s picture

Pancho
Location UTC+2 🇪🇺 EU
CreditAttribution: Pancho commented

Again, project.module won't let me upload my patch...

Pancho’s picture

Pancho
Location UTC+2 🇪🇺 EU
CreditAttribution: Pancho commented
FileSize
remove_top_bottom_buttons.patch5.5 KB

Ah, here we go.

Pancho’s picture

Pancho
Location UTC+2 🇪🇺 EU
CreditAttribution: Pancho commented
Title: Context table: no need to move upmost argument to the top » Context table: remove 'top' and 'bottom'
Priority: Minor » Normal
merlinofchaos’s picture

merlinofchaos CreditAttribution: merlinofchaos commented

Looks ok on a visual review. I'll see if I can get to this today.

sdboyer’s picture

sdboyer CreditAttribution: sdboyer commented
Status: Needs review » Fixed

Tested, thumbs-up. I'd commit it, but it seems to me this could/should be expanded to remove up arrows from items at the top of one of these lists, down arrows from items at the bottom, and both arrows from items that are alone?

That may be a bit more complicated, since it means calculating the contents of the whole list and adding/removing items as needed. I'm not particularly attached to the idea, it just seems like a wise decision in terms of good UI.

Pancho’s picture

Pancho
Location UTC+2 🇪🇺 EU
CreditAttribution: Pancho commented
Status: Fixed » Reviewed & tested by the community

Yeah, that'd be the optimum from the UI point of view and we should definitely target this for RC.
On my sandbox, I have also some more UI improvements for the context page, that I will roll into a patch, as soon as time permits.

Changing status to RTBC, as patch has not yet been committed.

Pancho’s picture

Pancho
Location UTC+2 🇪🇺 EU
CreditAttribution: Pancho commented
Component: Attn -- m » User interface
sdboyer’s picture

sdboyer CreditAttribution: sdboyer commented

Whoops, thanks, accidentally had marked it fixed, because I was initially gonna commit it, until I thought of the extra piece about removing the up/down buttons.

merlinofchaos’s picture

merlinofchaos CreditAttribution: merlinofchaos commented

Removing the extra buttons turns out to be quite a bit of work because you have to add placeholders to prevent the buttons from moving around. (It's very jarring if the buttons move).

Pancho’s picture

Pancho
Location UTC+2 🇪🇺 EU
CreditAttribution: Pancho commented
Status: Reviewed & tested by the community » Needs review
FileSize
remove_top_bottom_buttons.jpg68.52 KB
remove_top_bottom_buttons_16.patch6.37 KB

So I'd say, we should leave the inactive buttons in for now. There's so much to do and we need to move forward with this.

Enclosed patch also replaces the 'configure' and 'delete' buttons by the nicer ones used in the content screen. From my POV it looks a lot better, even though the arrows are not 100% the same style. Please check and RTBC if you like it as well.

Note: Whoever commits this, needs to remove 'add.gif', 'close.gif', 'configure.gif', 'delete.gif', 'go-bottom.png' and 'go-top.png'.

merlinofchaos’s picture

merlinofchaos CreditAttribution: merlinofchaos commented
Status: Needs review » Fixed

Applied. NOte that close.gif appears to still be in use.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.