Open core-ai-bot opened 3 years ago
Comment by peterflynn Wednesday Apr 15, 2015 at 23:42 GMT
Here's what the UI looks like now with this PR:
Comment by peterflynn Wednesday Apr 15, 2015 at 23:45 GMT
@
prksingh et al: regardless of whether we merge this or some other variation of PR #10887, we should probably do another test pass on this feature overall since the notification & the data-send timing is changing significantly from how it used to behave. (I tested this a bunch locally of course, but it's important to have a 2nd set of eyes).
Comment by prksingh Thursday Apr 16, 2015 at 04:46 GMT
This looks good@
peterflynn Thanks for the UI tweaks! I will take another look at this later and merge this.
@
prafulVaishnav Can you also have a look at the changes?
Comment by prafulVaishnav Thursday Apr 16, 2015 at 05:27 GMT
@
peterflynn If user have opt-out then we should not be showing the health data preview as in that case we will not be logging/sending anything. Instead we should show the preference dialog where he can opt-in.
Comment by peterflynn Thursday Apr 16, 2015 at 07:59 GMT
@
prafulVaishnav That change is by design -- if the user has opted out, it's still useful for them to be able to see what data would be sent if they were to opt back in. Maybe they'll decide it's not so scary after all, for example :-) The UI is now unified: the dialog contains both the preference and the preview.
Comment by prafulVaishnav Thursday Apr 16, 2015 at 08:10 GMT
@
peterflynn@
nethip We had some discussion on it based on which we decided to keep different dialog box for preference(to opt-in) and preview(containing option to opt-out) Previewing the data on opt-out will give the feeling to the user that we are still tracking the data which we aren't.
Comment by peterflynn Thursday Apr 16, 2015 at 08:48 GMT
It seems important for people to be able to see the data and then decide if they're ok with sending it. The Firefox one works this way too. Given that the checkbox is right there, do you think people would really misunderstand? @
ryanstewart, thoughts?
Comment by nethip Thursday Apr 16, 2015 at 09:18 GMT
@
peterflynn I think people would have more confidence when we show them a dialog saying we are not tracking your data anymore as you have opted out against showing them the data even they have opted out. Talking about the Firefox model, I personally would feel a little odd to see the tracking data though I have opted out of it.
Comment by peterflynn Thursday Apr 16, 2015 at 09:23 GMT
One of the points of the Firefox Health Report -- and hopefully here too -- is that the data can benefit the user directly by showing them stats about their performance, average project size, etc. So you don't want to refuse the user access to their own data just because they've opted out of sharing it with others. This is also useful for field diagnostics -- e.g. if a user complains Brackets is running slow, we could ask them to check the Health Report dialog and see if the projects they tend to edit are unusually large, etc.
What about having the string vary depending on opt-in state? E.g. when opted out, it could say something like: "Below is a preview of the data that would be sent in your next Health Report if enabled (sharing your Health Report is currently disabled)."
Comment by nethip Thursday Apr 16, 2015 at 09:32 GMT
@
peterflynn Yeah I am OK with the changing of the wording. We need to clearly distinguish between diagnostic data and tracking data. I am in for showing the former in case I opt out.
I can compare this to be a browser cookie just waiting to be used.
Comment by nethip Thursday Apr 16, 2015 at 09:53 GMT
That being said I am fine if we can express the same with good wording.
Comment by peterflynn Friday Apr 17, 2015 at 01:37 GMT
@
nethip Ok, I'll push up the dynamic wording changes tonight.
Comment by peterflynn Friday Apr 17, 2015 at 06:56 GMT
@
nethip It felt weird having the wording jump around as the checkbox is toggled, so I went with a more static set of wording that emphasizes the previewed data isn't sent if you've opted out:
(this also simplifies the checkbox label itself, after some discussion with NJ &@
ryanstewart)
@
MarcelGerber Thanks for the feedback -- changes pushed to fix those nits too
@
prafulVaishnav I agree we may want a more generalized Brackets notification UI widget in the future. Are you ok with waiting until a future PR to generalize this, though? It's good to have more concrete use cases in mind before generalizing anyway...
Comment by prafulVaishnav Friday Apr 17, 2015 at 09:05 GMT
@
peterflynn Yes in future we will take care of generalizing it. Thanks a lot Peter. Merging this.
Issue by peterflynn Wednesday Apr 15, 2015 at 23:39 GMT Originally opened as https://github.com/adobe/brackets/pull/10912
This is a superset of #10887.
@
prksingh sorry, I hope this doesn't step on anything you'd already started writing... I set out to just tweak the strings & CSS, and this sort of snowballed after that :-P I tried to split this cleanly into separate commits, so if you really wanted to take only the visual changes I think you could do that.Here are the differences from 10887:
@
ryanstewart &@
njx for feedback)HealthDataNotification.showDialogHealthDataNotification()
and change unit test to check notification popup insteadpeterflynn included the following code: https://github.com/adobe/brackets/pull/10912/commits