Skip to content
This repository was archived by the owner on Dec 23, 2021. It is now read-only.

Add privacy modals to external links #62

Merged
merged 10 commits into from
Jul 29, 2019
Merged

Conversation

LukeSlev
Copy link
Member

@LukeSlev LukeSlev commented Jul 18, 2019

Description:

This PR adds a privacy modal to external links in our app.

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

Limitations:

Due to limitations in the notification api, we can't put hyperlinks into the notifications so the user will have to copy and paste the privacy link themselves.

Testing:

  • The tutorials on adafruit link has the privacy modal
  • the I need help notification from device deployment has the privacy modal
  • the links on the notifications work as expected

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules

@FMounz FMounz changed the title Add privacy modals to extenal links Add privacy modals to external links Jul 19, 2019
Copy link
Contributor

@jonathanwangg jonathanwangg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The user flow for getting to the tutorial feels very weird.

e.g.) There is no "OK" button to proceed to the external website on the first modal.
e.g.) From the first modal, even after clicking cancel, the second modal still pops up.

@LukeSlev
Copy link
Member Author

@jonathanwangg made some changes have another look!

Copy link
Contributor

@jonathanwangg jonathanwangg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice changes. Thanks for the update!

Copy link
Contributor

@Christellah Christellah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small question, beside that tested and works as expected 👍

src/constants.ts Outdated
@@ -136,6 +139,8 @@ export namespace DialogResponses {
export const DONT_SHOW: MessageItem = {
title: localize("dialogResponses.dontShowAgain", "Don't Show Again")
};
export const ACCEPT_PRIVACY: MessageItem = { title: localize("dialogResponses.acceptPrivacy", "Got it") };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the PR 63 (https://github.com/microsoft/vscode-python-embedded/pull/63/files) about filename checks, there is another "Got it option" constant that's being added. Do we want to keep them separate ?

@LukeSlev LukeSlev merged commit c4e3ab5 into dev Jul 29, 2019
@LukeSlev LukeSlev deleted the users/t-luslev/add-privacy-modal branch August 7, 2019 23:27
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants