Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add support for the Kinto Account plugin. #439

Merged
merged 4 commits into from
Jun 23, 2017
Merged

Add support for the Kinto Account plugin. #439

merged 4 commits into from
Jun 23, 2017

Conversation

Natim
Copy link
Member

@Natim Natim commented Jun 23, 2017 •
edited

Fixes #436

@Natim Natim requested a review from n1k0 June 23, 2017 09:13
@Natim Natim changed the title Add support for the Kinto Account plugin. Fixes #436 Add support for the Kinto Account plugin. Jun 23, 2017
@Natim Natim force-pushed the 436-account-plugin-support branch from 554445d to 2aaecd3 Compare June 23, 2017 09:16
n1k0
n1k0 reviewed Jun 23, 2017
View reviewed changes
src/components/AuthForm.js Outdated
@@ -103,6 +103,36 @@ const baseUISchema = {
};

const authSchemas = {
account: {
Copy link
Contributor

@n1k0 n1k0 Jun 23, 2017 •
edited

Choose a reason for hiding this comment

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

Just me or this account schema is basically the basicauth one with just the title modified? If so we should rather create a common base and extend it appropriately.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is indeed.

src/actions/session.js
.href = `${server}/fxa-oauth/login?redirect=${encodeURIComponent(redirect)}`;
document.location.href = `${server}/fxa-oauth/login?redirect=${encodeURIComponent(
redirect
)}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious as to why this has been reformatted. Did you npm install to retrieve the latest version of prettier?

Copy link
Member Author

@Natim Natim Jun 23, 2017 •
edited

Choose a reason for hiding this comment

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

Yes, I even rimraf node_modules to make sure it was the one pinned in the package.json

Copy link
Member Author

Choose a reason for hiding this comment

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

do you want me to revert this specific change?

Copy link
Contributor

Choose a reason for hiding this comment

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

No it's ok

@n1k0
Copy link
Contributor

n1k0 commented Jun 23, 2017

Seems we have an issue with the prettier version you're using when running cs-format on your machine, as per travis job result:

> kinto-admin@1.13.3 cs-check /home/travis/build/Kinto/kinto-admin
> prettier-check $npm_package_prettierOptions '{src,test}/**/*.js'
Forgot to run prettier? There are files without correct code style:
src/actions/session.js
src/components/AuthForm.js
src/components/collection/CollectionForm.js
src/components/HistoryTable.js
src/components/HomePage.js
src/components/Notifications.js
src/components/Sidebar.js
src/components/TagsField.js
src/containers/collection/CollectionAttributesPage.js
src/containers/collection/CollectionPermissionsPage.js
src/permission.js
src/routes.js
src/utils.js
test/components/CollectionRecords_test.js

Could you please ensure you're running the exact same version required in package.json?

Also it seems a test is now breaking following a recent react upgrade:

1) App component Session top bar should not render a session top bar when not authenticated:
     Error: Warning: Failed prop type: The prop `params` is marked as required in `t`, but its value is `undefined`.
    in t (created by App)
    in App
      at Console.<anonymous> (test/test_utils.js:17:11)

Could you please fix this one as well so we have a green build?

Thanks!

@Natim
Copy link
Member Author

Natim commented Jun 23, 2017

Yes sure, thanks for the review.

@Natim
Copy link
Member Author

Natim commented Jun 23, 2017

I use:

  • prettier@1.4.4
  • prettier-check@1.0.0

I have no idea looking at the package.json file if it is good or not.

@Natim Natim force-pushed the 436-account-plugin-support branch from d3f4718 to 83d4f87 Compare June 23, 2017 15:20
@Natim
Copy link
Member Author

Natim commented Jun 23, 2017

Rebase with prettier 1.0.0

@Natim Natim merged commit 48e3c37 into master Jun 23, 2017
@Natim Natim deleted the 436-account-plugin-support branch June 23, 2017 15:59
@glasserc glasserc mentioned this pull request Jun 23, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@n1k0 n1k0 n1k0 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@Natim @n1k0