Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Dries doesn't like the revision overview screen.
1) there is a numbered column in the front of the table which he says does not make sense.
2) he does not like the way the log is output either.
3) he is not sure the title makes much sense.
I'd like to invite everbody to discuss this and I will rework the screen to match whatever consensus is reached.
Comment | File | Size | Author |
---|---|---|---|
#37 | drupal_42.patch | 2.72 KB | m3avrck |
#32 | drupal_40.patch | 8.07 KB | m3avrck |
#29 | rev_0.png | 8.46 KB | m3avrck |
#28 | drupal_39.patch | 7.6 KB | m3avrck |
#25 | rev.png | 9.12 KB | m3avrck |
Comments
Comment #1
killes@www.drop.org CreditAttribution: killes@www.drop.org commentedhere is a screenshot, feel free to annotate.
Comment #2
m3avrck CreditAttribution: m3avrck commentedWhere does the 'initial version' etc stuff come from, is that the log?
1. numbers make sense, revision 1, revision 2, etc ... maybe perhaps prefixing 'rev 1', 'rev 2' would make more sense?
2. looks ok to me
3. title makes sense because you can change the title revision to revision, perhaps a better screen shot showing this would have it make more sense?
I'm indifferent to revising this but I'd like to see what other people think too.
Comment #3
killes@www.drop.org CreditAttribution: killes@www.drop.org commentedyes, initial version etc is the log. A suggestion was to change the numbers to the date.
Comment #4
Stefan Nagtegaal CreditAttribution: Stefan Nagtegaal commentedI'll post a UI-review tomorrow night...
Comment #5
factoryjoe CreditAttribution: factoryjoe commentedSo is there a screenshot somewhere I can see?
Comment #6
factoryjoe CreditAttribution: factoryjoe commentedWhoops, hadn't refreshed before I commented. Check out the way Writeboard handles revisions *on the node* itself.
Comment #7
killes@www.drop.org CreditAttribution: killes@www.drop.org commentedDries, how do you want me to proceed?
Comment #8
Zen CreditAttribution: Zen commentedScreenshot attached. I've done away with "title", and replaced "(current)" with an asterisk. Not sure about the latter though..
Cheers,
-K
Comment #9
m3avrck CreditAttribution: m3avrck commentedI would switch that last one around, putting on the top line:
Just changing the focus to the revision and date with accompanying actions and putting the log message and author name as secondary (which they really are).
Comment #10
m3avrck CreditAttribution: m3avrck commentedAlso, I wouldn't make each row in the table seperated by lines (too confusing) but rather only use lines to seperate each revision (so every 2 rows).
Comment #11
killes@www.drop.org CreditAttribution: killes@www.drop.org commentedI'd like to point out that the log message can be longer than any of the examples used in the mockups. this should be considered.
Comment #12
m3avrck CreditAttribution: m3avrck commentedRight, so if we use the approach above I mentioned, if log messages are a few lines long and they wrap, it won't interfere with selecting revisions, especially if each revision is seperated instead of row, it will make the readability much better, IMO.
Comment #13
Dries CreditAttribution: Dries commentedThe screenshots look ugly/complex IMO. Why do we want to make multi-row table entries? It complicates things. Can someone try one where everything is on one line (of course, the message is allowed to wrap)?
The "view set active delete" is somewhat confusing as well, because there is no clear separation between different options (because the links are not underlined). Maybe 'set active' should become 'activate'?
Comment #14
eaton CreditAttribution: eaton commentedIf a given revision is set to 'active' will the revisions AFTER it be preserved, or discarded?
"Rollback" or "Restore" might be easier to grasp titles for the link than "set active." That's what a user is trying to do, after all -- restore the contents of an older revision.
Comment #15
m3avrck CreditAttribution: m3avrck commented@eaton yes older revisions are not discarded unless they are specifically deleted.
Here is a new screen where I put my earlier ideas into something visually pleasing.
A few caveats that you guys might come up with: to view older revision, click on the date (duh!) as this is generally the case, the clears up the operations, we can obviously add a 'view' link but that is too much clutter IMO.
Additionally, as Eaton suggested, 'set active' isn't usually used when working with revisions, 'rollback' or 'revert' tend to be better choices so I have gone ahead with that.
This really cleans things up and makes it easier to read.
@Dries notice how long log messages could be, this makes it easier to view if they wrap underneath instead of on one line. A lot of themes are fixed width and not too wide and having log messages on the same long could produce some extremely short length lines (1 or 2 words) having it below gives it more room to breath.
Comment #16
Zen CreditAttribution: Zen commented@m3avrck's layout. A lot cleaner and I like the fact that "view" is gone and the "by user" keeps things a lot more concise.
+1
Cheers :)
-K
Comment #17
m3avrck CreditAttribution: m3avrck commentedSlightly better one based on comments from IRC.
Comment #18
Zen CreditAttribution: Zen commentedCool :) Only thing is that the duplicate asterisk seems out of place. Perhaps the second one can go *poof*?
Also, I just realised that "Revert" (as Eaton mentioned) might be slightly inaccurate/misleading here, as newer revisions are still retained. It also implies "going back to something in the past" while that might not always be the case here. As Dries suggested, "Activate" might be more suitable..
But +1 except for these nit-picks :)
-K
Comment #19
Dries CreditAttribution: Dries commentedThe latest screenshots look a lot nicer! :)
Comment #20
m3avrck CreditAttribution: m3avrck commentedOk one last revision based on comments from above and IRC and off of how wikipedia works
Now, just to be clear, revisions should go in reverse chronological order (newest first), this is now clear in the screen shot.
Secondly, we should create a new CSS class, called
.revision-active
. Reason for this, is that many themes make use of alternating table row colors. With this new change, that is still supported, however to mark the current revision as active, we need another class so that this can stand out.Thirdly, the 'revert' issue. By defintion:
Now revisions work like this: I create revision 1, someone else doesn't like it, so they create revision 2, someone else creates revision 3 and so forth. As each revision is created, it is marked as 'current' as generally revisions mean improvements. So if revision 3 is current and we want to go back to revision 1, we revert to revision 1.
Of course, revision 1 is the oldest, but it is now the current revision. If we want revision 3 to be current, we go back to revision 3 (because despite it having the newest creation date, it is still an older revision now because revision 1 is current). Hence, we revert back to revision 3.
That is why I suggest staying with 'revert' I see it come up the most often when dealing with revisions.
As a note, I natively speak English but I'm by far, no English major ;-)
Comment #21
m3avrck CreditAttribution: m3avrck commentedforgot screen shot.
Comment #22
killes@www.drop.org CreditAttribution: killes@www.drop.org commentedJust a remark: If you revert to an old revision, there will a new revision be created which is a copy of the old one.
Comment #23
Zen CreditAttribution: Zen commented@m3avrck: Neat :) I'm OK with either, but thought Activate might be a more neutral expression :)
@killes: Ah, I hadn't noticed that :) Perhaps an anchor link [In the copied revision's log message] to the original revision might be handy?
OT - I also noticed that the watchdog messages don't seem to indicate any of the revision activity - they just say "page update ..".
Cheers :)
-K
Comment #24
m3avrck CreditAttribution: m3avrck commentedOk here's a patch to fix this, it accomplishes the following:
1. Cleans up the display to look like screen shot in #21
2. Changes all references from 'rollback' to 'revert' to be consisent with screen shot
3. Reverses node_revisions query for DESC instead of ASC (newest revisions should be at top)
Comment #25
m3avrck CreditAttribution: m3avrck commentedHere's a screen of what this looks like in bluemarine theme.
Comment #26
Dries CreditAttribution: Dries commentedClose but no cigar. ;-)
"Log message:" can't be translated.
t(' by ')
might not be a good idea. Probably better to uset('%date by %author', ...)
or something equivalent. You might want to check how other 'by's are translated elsewhere.There are some instances of 'rollback' left:
'%title has been rolled back to the revision from %revision-date'
Comment #27
Steven CreditAttribution: Steven commentedWhy do we need "Log Message:" in each entry at all? Isn't it clear from context? It just seems like clutter to me.
Comment #28
m3avrck CreditAttribution: m3avrck commentedDries, good call! It was late last night when I did the patch, here's a better one. Fixed all instances of the term 'roll' so everything is consistent now. Fixed the translatable 'by' string. And as Steve suggested, I removed the 'Log message:' all together. Take a look at the screen shot and compare to the one in comment in #25 ... whichever looks better we'll go with.
Comment #29
m3avrck CreditAttribution: m3avrck commentedAnd the screen with 'Log message' removed.
Comment #30
m3avrck CreditAttribution: m3avrck commentedComment #31
Dries CreditAttribution: Dries commentedDisplaying raw data ($revision->log) is vulnerable to XSS attacks, or is the log field checked/escaped elsewhere?
Comment #32
m3avrck CreditAttribution: m3avrck commentedDries, you're right. The code was that way before I made my patch, I've updated it accordingly. I also noticed that
$node->log
was wrongly being output in another spot too and I have fixed that in this updated patch.Comment #33
Dries CreditAttribution: Dries commentedCommitted to HEAD. Thanks.
Comment #34
Zen CreditAttribution: Zen commentedI really think that losing the log field is a _bad_ idea. If I have a 100 revisions, I will be totally lost without the log field. I personally already have pages with about 20 revisions in them, where this would prove invaluable. Sifting through them manually to find out which revision to revert to is not something I'm looking forward to.
Perhaps a compromise could be to just truncate the log message at 128 chars or something with a trailing ellipsis appended :)
I am also not too keen on the fact that copies are being made after each revert - it just seems very wasteful. There should be a more efficient solution for preserving the revision history of a node.. But, for the purposes of this issue, would it be a good idea to link the copy to the original via an anchor link? Again, this is to make things easier when the number of revisions listed is large.
Hope that made sense :)
-K
Comment #35
m3avrck CreditAttribution: m3avrck commentedZen, the log message is *not* gone at all, it is still there. We were refering to the *actual* text: 'Log message' preceeding the actual log message.
As for why copies of revisions are made, see my other thread: http://drupal.org/node/45071 ... I actually *now* agree that copies make more sense, as they show the flow of work on a node and you can easily diff changes.
Comment #36
Zen CreditAttribution: Zen commentedAh right - apologies for jumping the gun there. The "Copy of revision..." threw me off.
Diffs - ok, thanks - more reading :)
Thanks,
-K
Comment #37
m3avrck CreditAttribution: m3avrck commentedHmm, looks like that new CSS class to highlight the current revision as yellow didn't go in.
Also, realized that
check_plain()
was wrong. Misunderstood chx, should have usedfilter_xss()
so that any HTML in the log message can actually be rendered.New patch fixes both of these.
Comment #38
Dries CreditAttribution: Dries commentedCommitted. Thanks again.
Comment #39
(not verified) CreditAttribution: commented