I posted a patch, which failed)
Then I posted a new patch (forgetting to mark it needs review).
Then I edited the issue, marked it needs review, and unchecked "display" for the failed patch.

Expected:
The hidden patch would be hidden in the recent files table, and nothing but the status change would be in my comment.

Actual:
The hidden patch would be hidden in the recent files table, but the now-hidden file is shown in my status changing comment as though I uploaded it

CommentFileSizeAuthor
#16 2125741_nodechanges-file-status-changes-in-fieldset_v3.patch6.98 KBjthorson
#15 2125741_nodechanges-file-status-changes-in-fieldset_v2.patch7 KBjthorson
#14 2125741_nodechanges-file-status-changes-in-fieldset.patch6.63 KBjthorson
#12 uncollapsed-hidden-files.jpg114.41 KBmarkhalliwell
#12 collapsed-hidden-files.jpg113.57 KBmarkhalliwell
hidden-file-fail.png110.32 KBtim.plunkett
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented

https://drupal.org/comment/8136229#comment-8136229 is the issue in question.

eliza411’s picture

eliza411 CreditAttribution: eliza411 commented
Issue tags: +D.o UX
jthorson’s picture

jthorson CreditAttribution: jthorson commented

The intent was that the comment thread would show when a file was removed from the table, and who did it ... thus the status = 'hidden' instead of 'new'. So from a nodechanges perspective, this is 'works as designed'. There was a bit of debate during development as to whether PIFT should show test status for these 'hidden' / 'shown' entries; but for now, it does not.

Recognizing the potential for UX misunderstandings here, is there an proposed alternative for how we should handle these?

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented

Collapse them as well? It makes it really hard to tell what is happening.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Priority: Normal » Major

I'm tempted to make this critical but it is at least major. Very very confusing for people - at last person was about to mark an rtbc issue needs work because of this.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented

Here is the example in question: https://drupal.org/comment/8169585#comment-8169585

Perhaps hidden files could be listed like when you add/remove issue tags or related issues?
Just show the names, not all of the metadata, and not in line with the newly uploaded patches.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

How about this:

If a file is added, show all the meta-data as is currently done.

If a file is hidden, don't show the file as a link, don't show the size, and put the word "hidden" in a different color. Or maybe make the background of that row a different color? Just brainstorming...

herom’s picture

herom CreditAttribution: herom commented

here's another evidence:
https://drupal.org/comment/8134877#comment-8134877

the comments #16-#21 were all referring to an older patch I hid at #15.

YesCT’s picture

YesCT CreditAttribution: YesCT commented

#2102125-121: Big Local Task Conversion had a tiny confusion about this.

markhalliwell’s picture

markhalliwell
Primary language English
CreditAttribution: markhalliwell commented

FWIW, Dreditor 1.2.3 is now wrapping hidden and deleted files in collapsible fieldsets to prototype this and see if this is what we want to do. If so, we should make the change and then I can remove it from Dreditor.

ianthomas_uk’s picture

ianthomas_uk
he/him
Primary language English
Location Brighton, UK
CreditAttribution: ianthomas_uk commented

I think it is often useful to show the files that were hidden by an update, in much the same way that we show the old and new values when something like the status changes.

I agree that at the moment it looks like you're uploading a new file though. Could we display the hidden files in grey text with a strikethrough?

markhalliwell’s picture

markhalliwell
Primary language English
CreditAttribution: markhalliwell commented
FileSize
collapsed-hidden-files.jpg113.57 KB
uncollapsed-hidden-files.jpg114.41 KB

For those who don't use Dreditor:
Collapsed:
collapsed-hidden-files
Un-collapsed:
uncollapsed-hidden-files

jthorson’s picture

jthorson CreditAttribution: jthorson commented

Looked at this last night ...
- Grey and strikethrough would be fairly trivial, but nodechanges doesn't have its own .css file.
- Moving it into the box with the rest of the summary would take a major refactoring
- Adding the files to a hidden fieldset shouldn't be too difficult, but I'd probably make it a configurable behavior on the nodechanges field formatter, which does mean a bit more complexity.

jthorson’s picture

jthorson CreditAttribution: jthorson commented
Status: Active » Needs review
FileSize
2125741_nodechanges-file-status-changes-in-fieldset.patch6.63 KB
jthorson’s picture

jthorson CreditAttribution: jthorson commented
FileSize
2125741_nodechanges-file-status-changes-in-fieldset_v2.patch7 KB

Made 'fieldset' the default setting, and put a placeholder TODO in to handle the 'exclude' setting when there are no other changes (not relevant for the 'fieldset' setting to be used on drupal.org).

Deployed and tested on -dev site. Conflicts with current dreditor (causes nested fieldsets), so we'll need to pull the functionality out of there once this is deployed.

jthorson’s picture

jthorson CreditAttribution: jthorson commented
FileSize
2125741_nodechanges-file-status-changes-in-fieldset_v3.patch6.98 KB

Had an extra $summary = ''; in there.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented

One useful thing, in case dreditor or a userstyle want to improve on this, is to have a CSS class denoting whether something is new, hidden, or deleted in the row itself. Currently dreditor has to check the actual text contents of the td, which is very very slow.

markhalliwell’s picture

markhalliwell
Primary language English
CreditAttribution: markhalliwell commented

This functionality has been removed from Dreditor https://github.com/dreditor/dreditor/commit/5af2544183139bb1ce38ad8245db...

So the next release of Dreditor is now tied to this issue becoming implemented and deployed.

dww’s picture

dww
we/he/they
CreditAttribution: dww commented
Status: Needs review » Fixed

Nice improvement to the UX! Love it.

Committed and pushed your patch as-is:
http://drupalcode.org/project/nodechanges.git/commit/ba89c3e

In careful review, I found some small problems: a few bugs, some harder-to-understand code paths and names, and some other cosmetic stuff. Fixed all that up, and added the class tim.plunkett requested in #17:

http://drupalcode.org/project/nodechanges.git/commit/7cbea66

Merged into bzr and deployed live. Hurray!

Thanks everyone,
-Derek

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

Nice. I like that we can just scroll up to comment #16 and see the result. Looks great!

Status: Fixed » Closed (fixed)

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