|
|
|
Created:
15 years, 1 month ago by Ilia Mirkin Modified:
15 years, 1 month ago CC:
codereview-discuss_googlegroups.com Visibility:
Public. |
Hook up implementation of showing a mildly prettified unified diff on the issue page directly, make sure that the 'i' key goes to that rather than the patch page. [The latter is still available via the remaining link]
Patch Set 1 #Patch Set 2 : Fix link logic to use onclick instead of href #
Total messages: 6
|
Ilia Mirkin
http://37.latest.rietveld-test.appspot.com/1/ Which actually includes all of the changes I've sent out thus far. Note that ...
|
15 years, 1 month ago (2010年11月13日 21:36:04 UTC) #1 | |||||||||||||||||||||||||||||||
http://37.latest.rietveld-test.appspot.com/1/ Which actually includes all of the changes I've sent out thus far. Note that this changes the behaviour of 'i'. I'd be OK with taking that out if people felt strongly, or making it a user configurable option. I'm not a huge fan of the "(show)" thing, but I didn't want to remove the commentable unified diff view entirely, since I assume it was there for a reason. I've had to comment out some code which was designed around a "show all unified diffs" feature which is a little harder to implement given that there are multiple patchsets, etc. [But it's not like that code ever got called in Rietveld...]
In the demo the (show) link gives me a page that says "false". Surely something's not right there? There are definitely situations where the commentable unified view is right. (It would be nice if it got the pretty file-based urls you did for the side-by-side diff.) I agree that (show) is ugly. I wonder if we should do something more radical like changing the "Unified diffs" and "side-by-side diffs" column headings so that the sbs diff is first and linked to the filename. Then in the unified column we could have two links, "view" and "inline". (Or maybe a different word for "view" to remind people of the UI change. But what?)
On Wed, Nov 17, 2010 at 11:47 AM, <gvanrossum@gmail.com> wrote: > In the demo the (show) link gives me a page that says "false". Surely > something's not right there? What says "false"? It works fine for me (Chrome/Linux, but pretty sure I've also tested this in Firefox/Linux, and the code that backs this is the super-original code that was copied from s back in the day). Browser/platform? Perhaps I messed up some silly integration point with the code that was already in place? > > There are definitely situations where the commentable unified view is > right. (It would be nice if it got the pretty file-based urls you did > for the side-by-side diff.) I can throw it on my list. In my local repo, that view is gone, so I don't have a patch I can just cherry-pick out :) > > I agree that (show) is ugly. I wonder if we should do something more > radical like changing the "Unified diffs" and "side-by-side diffs" > column headings so that the sbs diff is first and linked to the > filename. Then in the unified column we could have two links, "view" > and "inline". (Or maybe a different word for "view" to remind people of > the UI change. But what?) In my local repo, the "unified diff" view is gone completely, and it just says "filename (show)" with the show bringing out the view that I've hooked up to 'i'. It's less than ideal in terms of discovery, as you point out. But really it's really basically the same view as the commentable unified view... perhaps I can figure out a way to iframe the commentable view in there so that you have the full commenting capability? (Except that the javascript will probably collide... ugh. That's what we get for not using namespaces.) How about a note above each patchset that just explains it for now? -- Ilia Mirkin imirkin@alum.mit.edu
On Wed, Nov 17, 2010 at 9:01 AM, Ilia Mirkin <imirkin@alum.mit.edu> wrote: > On Wed, Nov 17, 2010 at 11:47 AM, <gvanrossum@gmail.com> wrote: >> In the demo the (show) link gives me a page that says "false". Surely >> something's not right there? > > What says "false"? It works fine for me (Chrome/Linux, but pretty sure > I've also tested this in Firefox/Linux, and the code that backs this > is the super-original code that was copied from s back in the day). > Browser/platform? Perhaps I messed up some silly integration point > with the code that was already in place? Maybe it's unique to FF. I have FF 3.6.12 on a Mac (OS 10.5.8) and when I click on the "(show)" link it takes me to a new nearly-empty page saying "false". It works correctly on Chrome, so I guess it's a JS issue. >> There are definitely situations where the commentable unified view is >> right. (It would be nice if it got the pretty file-based urls you did >> for the side-by-side diff.) > > I can throw it on my list. In my local repo, that view is gone, so I > don't have a patch I can just cherry-pick out :) Just keep that view as it is for now. >> I agree that (show) is ugly. I wonder if we should do something more >> radical like changing the "Unified diffs" and "side-by-side diffs" >> column headings so that the sbs diff is first and linked to the >> filename. Then in the unified column we could have two links, "view" >> and "inline". (Or maybe a different word for "view" to remind people of >> the UI change. But what?) > > In my local repo, the "unified diff" view is gone completely, and it > just says "filename (show)" with the show bringing out the view that > I've hooked up to 'i'. If you change (show) to (inline) that'll be better. > It's less than ideal in terms of discovery, as > you point out. But really it's really basically the same view as the > commentable unified view... Except it's not commentable and doesn't show comments. > perhaps I can figure out a way to iframe > the commentable view in there so that you have the full commenting > capability? (Except that the javascript will probably collide... ugh. > That's what we get for not using namespaces.) I hate iframes. I think that having the inline view not commentable is fine; adding draft comments to this view probably makes it too busy anyway. > How about a note above each patchset that just explains it for now? What would the note say? In general I hate UIs that require users to read sentences to figure out what they can do. -- --Guido van Rossum (python.org/~guido)
On Wed, Nov 17, 2010 at 2:46 PM, Guido van Rossum <guido@python.org> wrote: > On Wed, Nov 17, 2010 at 9:01 AM, Ilia Mirkin <imirkin@alum.mit.edu> wrote: >> On Wed, Nov 17, 2010 at 11:47 AM, <gvanrossum@gmail.com> wrote: >>> In the demo the (show) link gives me a page that says "false". Surely >>> something's not right there? >> >> What says "false"? It works fine for me (Chrome/Linux, but pretty sure >> I've also tested this in Firefox/Linux, and the code that backs this >> is the super-original code that was copied from s back in the day). >> Browser/platform? Perhaps I messed up some silly integration point >> with the code that was already in place? > > Maybe it's unique to FF. I have FF 3.6.12 on a Mac (OS 10.5.8) and > when I click on the "(show)" link it takes me to a new nearly-empty > page saying "false". It works correctly on Chrome, so I guess it's a > JS issue. Problem debugged and has a known fix. Turns out it probably only worked in Chrome due to some silliness on my part. > >>> There are definitely situations where the commentable unified view is >>> right. (It would be nice if it got the pretty file-based urls you did >>> for the side-by-side diff.) >> >> I can throw it on my list. In my local repo, that view is gone, so I >> don't have a patch I can just cherry-pick out :) > > Just keep that view as it is for now. Done :) > >>> I agree that (show) is ugly. I wonder if we should do something more >>> radical like changing the "Unified diffs" and "side-by-side diffs" >>> column headings so that the sbs diff is first and linked to the >>> filename. Then in the unified column we could have two links, "view" >>> and "inline". (Or maybe a different word for "view" to remind people of >>> the UI change. But what?) >> >> In my local repo, the "unified diff" view is gone completely, and it >> just says "filename (show)" with the show bringing out the view that >> I've hooked up to 'i'. > > If you change (show) to (inline) that'll be better. OK >> How about a note above each patchset that just explains it for now? > > What would the note say? In general I hate UIs that require users to > read sentences to figure out what they can do. Nothing that you'd like to see :) Something awkward like """click on "inline" to see a unified diff on this page, click on the filename for a commentable view""" -- Ilia Mirkin imirkin@alum.mit.edu
http://37.latest.rietveld-test.appspot.com/1/ New code uploaded which fixes the bug that you saw with Firefox. Not sure how the old code was supposed to work exactly... must have had something cleverer in the href.