|
|
Created:
12 years, 2 months ago by rch Modified:
1 week, 3 days ago CC:
codereview-discuss_googlegroups.com Visibility:
Public. |
DescriptionAdd an account setting to opt-in to new UI features.
Patch Set 1 #
Total comments: 2
Patch Set 2 : Fix comments... #
Total comments: 1
MessagesTotal messages: 26
How does this look?
Sign in to reply to this message.
I'd add the codereview-discuss@googlegroups.com to the CC list and M-A to reviewers since this will land on the default branch :) https://codereview.appspot.com/9971043/diff/1/templates/user.html File templates/user.html (right): https://codereview.appspot.com/9971043/diff/1/templates/user.html#newcode27 templates/user.html:27: {%if use_experimental_ui%} would it make sense to break this whole section out into a user_include_experimental.html, and then a user_include.html and simply switch between two {% include %} directives?
Sign in to reply to this message.
I've addressed your comment and have not seen other comments. When would you like to land? https://codereview.appspot.com/9971043/diff/1/templates/user.html File templates/user.html (right): https://codereview.appspot.com/9971043/diff/1/templates/user.html#newcode27 templates/user.html:27: {%if use_experimental_ui%} On 2013/06/03 19:03:40, iannucci wrote: > would it make sense to break this whole section out into a > user_include_experimental.html, and then a user_include.html and simply switch > between two {% include %} directives? Done.
Sign in to reply to this message.
+codereview-discuss@googlegroups.com I didn't see the discuss group on the CC? Anyhow, this LGTM. I'll land it, but there's a change blocking the chromium branch at the moment. I'll land as soon as that clears up. https://codereview.appspot.com/9971043/diff/16001/templates/user_experimental... File templates/user_experimental_include.html (right): https://codereview.appspot.com/9971043/diff/16001/templates/user_experimental... templates/user_experimental_include.html:1: <tr> I think this and the other include file can de de-indented by 2 spaces?
Sign in to reply to this message.
+maruel I'd like to get an LGTM on this from an owner :)
Sign in to reply to this message.
On 2013/06/07 17:40:42, iannucci wrote: > +maruel I'd like to get an LGTM on this from an owner :) Isn't that overkill? I mean, that's why AppEngine supports multiple concurrent simultaneous versions.
Sign in to reply to this message.
On 2013/06/07 17:42:42, M-A Ruel wrote: > On 2013/06/07 17:40:42, iannucci wrote: > > +maruel I'd like to get an LGTM on this from an owner :) > > Isn't that overkill? I mean, that's why AppEngine supports multiple concurrent > simultaneous versions. The purpose is so that we can improve and iterate on the UI without incurring the wrath of people who are very sensitive to UI changes, especially when those UI changes are WIP :). It also lets us surreptitiously gather data about who actually is using the new stuff v. the old stuff and lets us transition over to the new UI after 'perfecting' it (e.g. sorting CLs v. bolding them, adding a button to un-bold an issue explicitly, etc.).
Sign in to reply to this message.
On 2013/06/07 17:49:19, iannucci wrote: > On 2013/06/07 17:42:42, M-A Ruel wrote: > > On 2013/06/07 17:40:42, iannucci wrote: > > > +maruel I'd like to get an LGTM on this from an owner :) > > > > Isn't that overkill? I mean, that's why AppEngine supports multiple concurrent > > simultaneous versions. > > The purpose is so that we can improve and iterate on the UI without incurring > the wrath of people who are very sensitive to UI changes, especially when those > UI changes are WIP :). > > It also lets us surreptitiously gather data about who actually is using the new > stuff v. the old stuff and lets us transition over to the new UI after > 'perfecting' it (e.g. sorting CLs v. bolding them, adding a button to un-bold an > issue explicitly, etc.). That's to be committed in the chromium branch, right?
Sign in to reply to this message.
On 2013/06/07 18:02:54, M-A Ruel wrote: > On 2013/06/07 17:49:19, iannucci wrote: > > On 2013/06/07 17:42:42, M-A Ruel wrote: > > > On 2013/06/07 17:40:42, iannucci wrote: > > > > +maruel I'd like to get an LGTM on this from an owner :) > > > > > > Isn't that overkill? I mean, that's why AppEngine supports multiple > concurrent > > > simultaneous versions. > > > > The purpose is so that we can improve and iterate on the UI without incurring > > the wrath of people who are very sensitive to UI changes, especially when > those > > UI changes are WIP :). > > > > It also lets us surreptitiously gather data about who actually is using the > new > > stuff v. the old stuff and lets us transition over to the new UI after > > 'perfecting' it (e.g. sorting CLs v. bolding them, adding a button to un-bold > an > > issue explicitly, etc.). > > That's to be committed in the chromium branch, right? Well... It was to be committed to both. But it would make sense to change the default to 'on' for default and 'off' for chromium. Otherwise the merge conflicts will be crazy.
Sign in to reply to this message.
2013/6/7 <maruel@google.com>: > On 2013/06/07 17:49:19, iannucci wrote: >> >> On 2013/06/07 17:42:42, M-A Ruel wrote: >> > On 2013/06/07 17:40:42, iannucci wrote: >> > > +maruel I'd like to get an LGTM on this from an owner :) >> > >> > Isn't that overkill? I mean, that's why AppEngine supports multiple > > concurrent >> >> > simultaneous versions. > > >> The purpose is so that we can improve and iterate on the UI without > > incurring >> >> the wrath of people who are very sensitive to UI changes, especially > > when those >> >> UI changes are WIP :). > > >> It also lets us surreptitiously gather data about who actually is > > using the new >> >> stuff v. the old stuff and lets us transition over to the new UI after >> 'perfecting' it (e.g. sorting CLs v. bolding them, adding a button to > > un-bold an >> >> issue explicitly, etc.). > > > That's to be committed in the chromium branch, right? > > https://codereview.appspot.com/9971043/ > > -- > You received this message because you are subscribed to the Google Groups > "codereview-discuss" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to codereview-discuss+unsubscribe@googlegroups.com. > To post to this group, send email to codereview-discuss@googlegroups.com. > Visit this group at http://groups.google.com/group/codereview-discuss?hl=en. > For more options, visit https://groups.google.com/groups/opt_out. > >
Sign in to reply to this message.
On 2013/06/07 18:33:20, iannucci wrote: > On 2013/06/07 18:02:54, M-A Ruel wrote: > > On 2013/06/07 17:49:19, iannucci wrote: > > > On 2013/06/07 17:42:42, M-A Ruel wrote: > > > > On 2013/06/07 17:40:42, iannucci wrote: > > > > > +maruel I'd like to get an LGTM on this from an owner :) > > > > > > > > Isn't that overkill? I mean, that's why AppEngine supports multiple > > concurrent > > > > simultaneous versions. > > > > > > The purpose is so that we can improve and iterate on the UI without > incurring > > > the wrath of people who are very sensitive to UI changes, especially when > > those > > > UI changes are WIP :). > > > > > > It also lets us surreptitiously gather data about who actually is using the > > new > > > stuff v. the old stuff and lets us transition over to the new UI after > > > 'perfecting' it (e.g. sorting CLs v. bolding them, adding a button to > un-bold > > an > > > issue explicitly, etc.). > > > > That's to be committed in the chromium branch, right? > > Well... It was to be committed to both. But it would make sense to change the > default to 'on' for default and 'off' for chromium. > > Otherwise the merge conflicts will be crazy. M-A: *ping*
Sign in to reply to this message.
On 2013/06/07 18:33:20, iannucci wrote: > On 2013/06/07 18:02:54, M-A Ruel wrote: > > That's to be committed in the chromium branch, right? > > Well... It was to be committed to both. But it would make sense to change the > default to 'on' for default and 'off' for chromium. > > Otherwise the merge conflicts will be crazy. Wait, you mean the default branch gets all the experimental stuff enabled by default and the chromium branch doesn't get these?
Sign in to reply to this message.
On 2013/06/17 23:54:44, M-A wrote: > On 2013/06/07 18:33:20, iannucci wrote: > > On 2013/06/07 18:02:54, M-A Ruel wrote: > > > That's to be committed in the chromium branch, right? > > > > Well... It was to be committed to both. But it would make sense to change the > > default to 'on' for default and 'off' for chromium. > > > > Otherwise the merge conflicts will be crazy. > > Wait, you mean the default branch gets all the experimental stuff enabled by > default and the chromium branch doesn't get these? The 'experimental' stuff is the interface which has already been deployed (i.e. what you see when you go to your own issues page). Some ppl on chromium-dev would like it to be better before deploying. I was assuming it would be on for default since the people on default seem to like it.... I have no preference either way.
Sign in to reply to this message.
2013/6/18 <rch@chromium.org> > On 2013/06/07 18:33:20, iannucci wrote: > >> On 2013/06/07 18:02:54, M-A Ruel wrote: >> > On 2013/06/07 17:49:19, iannucci wrote: >> > > On 2013/06/07 17:42:42, M-A Ruel wrote: >> > > > On 2013/06/07 17:40:42, iannucci wrote: >> > > > > +maruel I'd like to get an LGTM on this from an owner :) >> > > > >> > > > Isn't that overkill? I mean, that's why AppEngine supports >> > multiple > >> > concurrent >> > > > simultaneous versions. >> > > >> > > The purpose is so that we can improve and iterate on the UI >> > without > >> incurring >> > > the wrath of people who are very sensitive to UI changes, >> > especially when > >> > those >> > > UI changes are WIP :). >> > > >> > > It also lets us surreptitiously gather data about who actually is >> > using the > >> > new >> > > stuff v. the old stuff and lets us transition over to the new UI >> > after > >> > > 'perfecting' it (e.g. sorting CLs v. bolding them, adding a button >> > to > >> un-bold >> > an >> > > issue explicitly, etc.). >> > >> > That's to be committed in the chromium branch, right? >> > > Well... It was to be committed to both. But it would make sense to >> > change the > >> default to 'on' for default and 'off' for chromium. >> > > Otherwise the merge conflicts will be crazy. >> > > M-A: *ping* > > https://codereview.appspot.**com/9971043/<https://codereview.appspot.com/9971... >
Sign in to reply to this message.
Sorry, I didn't followed the whole discussion. What's the long-term purpose of this patch? For now it only seems to make the CSS class "issue updated" toggleable. I suppose there's more in the long run?
Sign in to reply to this message.
On 2013/06/18 19:13:57, Andi wrote: > Sorry, I didn't followed the whole discussion. What's the long-term purpose of > this patch? For now it only seems to make the CSS class "issue updated" > toggleable. I suppose there's more in the long run? Essentially, there's some bikeshedding on chromium-dev about the recent rietveld interface changes. The purpose of this patch is to hide new UI changes (and future developments thereof) behind an opt-in boolean so that changes don't upset people while we perfect them. It also allows us to inspect the DB and get a 'real' poll of who is using the new interface v. those who aren't. The only reason this would need to go into default is so that we can continue to develop the new UI on both branches without having nightmare merging conflicts.
Sign in to reply to this message.
On Tue, Jun 18, 2013 at 9:18 PM, <iannucci@chromium.org> wrote: > On 2013/06/18 19:13:57, Andi wrote: >> >> Sorry, I didn't followed the whole discussion. What's the long-term > > purpose of >> >> this patch? For now it only seems to make the CSS class "issue > > updated" >> >> toggleable. I suppose there's more in the long run? > > > Essentially, there's some bikeshedding on chromium-dev about the recent > rietveld interface changes. The purpose of this patch is to hide new UI > changes (and future developments thereof) behind an opt-in boolean so > that changes don't upset people while we perfect them. It also allows us > to inspect the DB and get a 'real' poll of who is using the new > interface v. those who aren't. Interesting! What are the key arguments (I'm not on chromium-dev)? At first I was a bit confused too when I first saw the UI changes in production. And I'm still not totally convinced that they work as I'd expected them to work. However they point in the right direction and I like your phrase "while we perfect them" :) oh, and my "not totally convinced" may be the same reason as for the bikeshedding on chromium-dev: I have no concrete point. It's just the issues show up as updated where I've already read the comment. What we really need to do is to track a "read/unread" status and I'd like to use labels for that. > > The only reason this would need to go into default is so that we can > continue to develop the new UI on both branches without having nightmare > merging conflicts. It's not thought-out, but what about marking different topics with different tags then? For example the update/unread/new improvements to the issue list could have a tag like "experimental:updatemark" (instead just "experimental"). That would make it much easier to officially move a feature from experimental to stable. Mainly because we can just grep the code for such a tag to remove it. The downside is that we would have to include additional CSS files then or work out a naming scheme that could easily be refactored if a experimental feature goes live. -- Andi > > https://codereview.appspot.com/9971043/
Sign in to reply to this message.
On my side, I think it's more a naming issue; it's not experimental if it's on by default on the default branch. It's just "new vs old".
Sign in to reply to this message.
Sign in to reply to this message.
irrelevant
Sign in to reply to this message.
Sign in to reply to this message.
irrelevant
Sign in to reply to this message.
irrelevant
Sign in to reply to this message.
irrelevant
Sign in to reply to this message.
irrelevant
Sign in to reply to this message.
|