Skip to content
This repository has been archived by the owner on Jul 20, 2021. It is now read-only.

Initial implementation #1

Merged
merged 39 commits into from
Dec 19, 2019
Merged

Initial implementation #1

merged 39 commits into from
Dec 19, 2019

Conversation

drazik
Copy link
Contributor

@drazik drazik commented Dec 17, 2019

This is the initial implementation of cozy-passwords, as an onboarding app for the passwords extension.

When the user lands on the app, we introduce him this passwords thing:

image

If his browser doesn't support the extension, we tell him:
image

On next step, we ask him to improve his current password, with some advices:

image

If he accepts, he is redirected to cozy-settings. Then redirected back to cozy-passwords after updating his password (as implemented in cozy/cozy-settings#269). If he denies, we ask him to at least set a password hint:

image

Then we suggest him to install the browser extension:
image

And when the extension has been installed, we show a success page (the extension should add an attribute on the app's root node, which is not currently implemented):
image

Illustrations are chrome-based even if we are in Firefox. We will fix it soon to have Firefox illustrations in Firefox, and Chrome illustrations in Chrome.

.editorconfig Outdated Show resolved Hide resolved
.github/PULL_REQUEST_TEMPLATE Outdated Show resolved Hide resolved
.github/PULL_REQUEST_TEMPLATE Outdated Show resolved Hide resolved
.travis.yml Show resolved Hide resolved
.tx/config Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
manifest.webapp Outdated Show resolved Hide resolved
src/components/SecurityPage.jsx Show resolved Hide resolved
src/doctypes/todos.js Outdated Show resolved Hide resolved
[twitter]: https://twitter.com/cozycloud
[nvm]: https://github.com/creationix/nvm
[ndenv]: https://github.com/riywo/ndenv
[jest]: https://facebook.github.io/jest/
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I would advise to remove completely this file, it is always an out of date copy paste of other files.

src/styles/index.css Outdated Show resolved Hide resolved
src/components/SecurityPage.jsx Outdated Show resolved Hide resolved
src/components/SecurityPage.jsx Outdated Show resolved Hide resolved
@@ -33,6 +33,7 @@
data-cozy-app-slug="{{.AppSlug}}"
data-cozy-app-name-prefix="{{.AppNamePrefix}}"
data-cozy-app-editor="{{.AppEditor}}"
data-cozy-subdomain-type="{{.SubDomain}}"
data-cozy-icon-path="{{.IconPath}}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: We have a new template attribute available from the stack, containing all the data, we might as well use it. See cozy/cozy-stack#2270. I hope this is deployed ;)

src/components/HintPage.jsx Outdated Show resolved Hide resolved
src/components/PresentationPage.jsx Outdated Show resolved Hide resolved
const { className, ...rest } = props
return <div className={cx('Wrapper', className)} {...rest} />
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Should this thing be moved to cozy-ui ? Wrapper is too generic a name for me here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually it's like the existing NarrowContent, but a little less narrow, since it's max 768px wide, where NarrowContent is max 512px wide. Maybe a variant of NarrowContent?

src/components/SecurityPage.jsx Outdated Show resolved Hide resolved
src/components/SecurityPage.jsx Outdated Show resolved Hide resolved
<ListItem>
<Bold tag="strong">Privilégiez un mot de passe fort :</Bold>
Un mot de passe long avec des chiffres et des caractères
spéciaux sera impossible à casser.
Copy link
Contributor

Choose a reason for hiding this comment

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

Rien n'est impossible :/

const DumbInstallationPage = props => {
const { t, client } = props

const cozyURL = new URL(client.getStackClient().uri)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: We should a helper for this directly on the client. client.getURL() ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would save a few keystrokes and make code easier to read, yes 👍

yarn.lock Show resolved Hide resolved
html,
body {
height: 100%;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should it be only on mobile ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's mainly here for mobile purpose, but it doesn't affect desktop. Do you have any issue in mind?

"descriptionStart": "Add",
"descriptionEnd": "to your browser to automatically fill and save your passwords while browsing",
"descriptionStart": "Add the",
"descriptionEnd": "to your browser. This installation is necessary to allow you to automatically fill in and save your passwords while browsing.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: AMHA, when we need to fill some part of a sentence, we should not have several parts in the translation files, but a single one, here "description" with a %{variable} in it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem here is that we want to insert a react component in the middle of the sentence.

src/locales/fr.json Outdated Show resolved Hide resolved
Copy link
Contributor

@ptbrowne ptbrowne left a comment

Choose a reason for hiding this comment

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

LGTM, some nits and suggestions. Thanks for the short commits but AMHA some commits could have been merged with precedent commits for easier reviewing. Here I commented several times on things that were fixed later on. Thanks !

@drazik drazik merged commit 0d8ad10 into master Dec 19, 2019
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.

3 participants