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

Feature/restructure settings module #1478

Merged
merged 15 commits into from
Sep 5, 2016
Merged

Feature/restructure settings module #1478

merged 15 commits into from
Sep 5, 2016

Conversation

brylie
Copy link
Contributor

@brylie brylie commented Sep 2, 2016 •
edited

Closes #1474

Changes

  • Import Settings collection in settings file(s)
  • Create template helper for settingsCollection, used by AutoForm
  • Cleanup AutoForm hooks
  • Remove unneeded Settings schema fields
  • Automatic linting
  • Remove unused API Umbrella methods

@brylie brylie added this to the Sprint 30 milestone Sep 2, 2016
@bajiat
Copy link
Contributor

bajiat commented Sep 2, 2016

@marla-singer Would you be available to review Brylie's PR? @brylie deleted the previous one you were reviewing, because it was giving console errors. This should not have the errors.

@marla-singer
Copy link
Contributor

@bajiat Yes, i'm going to review it now

marla-singer
marla-singer reviewed Sep 2, 2016
View reviewed changes
settings/client/autoform.js
@@ -0,0 +1,5 @@
AutoForm.addHooks(['settings'], {
onSuccess () {
FlashMessages.sendSuccess('Settings saved.');
Copy link
Contributor

Choose a reason for hiding this comment

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

@brylie Add i18n token

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's switch to sAlert notifications only. Now we have both FlashMessages and sAlert.
For this PR is enough to change it to sAlert just here, but we need have a separate task to completely remove FlashMessages package from the project.

@marla-singer
Copy link
Contributor

@brylie It works good

55
55 reviewed Sep 2, 2016
View reviewed changes
settings/client/autoform.js
@@ -0,0 +1,5 @@
AutoForm.addHooks(['settings'], {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use AutoForm.hooks instead of AutoForm.addHooks.

@brylie
Copy link
Contributor Author

brylie commented Sep 5, 2016 •
edited

@marla-singer and @NNN I made the suggested changes. Please review ASAP.

Ping @bajiat.

@marla-singer marla-singer merged commit 2898753 into develop Sep 5, 2016
@brylie brylie removed the in progress label Sep 5, 2016
@brylie brylie deleted the feature/restructure-settings-module branch September 5, 2016 08:31
@ilarimikkonen ilarimikkonen added this to In Review in apinf/platform Apr 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@marla-singer marla-singer

Labels
Ready for review
Projects
apinf/platform
In Review
Milestone
Sprint 30
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@brylie @bajiat @marla-singer @55