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.

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

Store preferred language for resources (closes #93) #103

Merged
merged 28 commits into from
Nov 4, 2019
Merged

Conversation

mattstauffer
Copy link
Member

@mattstauffer mattstauffer commented Oct 23, 2019

Replaces/continues #94, closes #93 -- moved modifications to that PR into here so it could be a branch I have access to.

@Treggats, I started work on revising your PR. I paused when I saw that you were storing the user preference as an int; I'd rather we store them as strings. I'm going to pause for now, but next steps as I see it:

  • Change user preferences to be stored as strings
  • Move preferences to be a JSON column on the user. Consider something like https://github.com/Grimthorr/laravel-user-settings (but maybe it's better to just write it ourselves?)
  • Matt review the preferences page
  • Add the preferences back to the module page, as per my original mockups, and build them in Vue
  • Add tests around PreferenceController
  • Fix for non-logged-in user
  • Add the logic that merges user settings and session settings when it comes to resource language
  • Work through the insane number of @todos I introduced
  • Decide: should the resource switcher really just be a link instead of a vue component?

@mattstauffer mattstauffer self-assigned this Oct 24, 2019
@mattstauffer
Copy link
Member Author

Hitting pause for the night, but just in case anyone else takes a look, I moved preferences to be a JSON column on the user, and added a convenience method on the user named preferences().

It functions like the global config() helper; pass it a key and you get that key. Pass it a key and a value, you get that key and if it's not set you get the value. Pass it an associative array and it saves the contents of that array.

However, there's one big difference; it has defaults pre-set for each key, and you can only set keys that are in the pre-set list of preferences. So as we add a preference, we'll add its key, it's possible options, and its default into that list.

@m50 m50 mentioned this pull request Oct 27, 2019
3 tasks
@mattstauffer
Copy link
Member Author

I have decided to keep the resource switcher in Vue for now because it's the cleanest way to POST to the endpoint making the change.

Copy link
Contributor

@wotta wotta left a comment

Choose a reason for hiding this comment

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

I took the liberty to go through this pr because I noticed it had "a lot" of files.

Commented on some files with questions and suggestions. I also have a question about the public app.css and app.js.
Should those files be included to git? Reason is that it feels really weird to me to have "compiled" files in git.

I would also suggest to try to keep the pr a little bit smaller so it is more easy to go through the changes that were made.

app/Facades/Localization.php Show resolved Hide resolved
app/Http/Controllers/PreferenceController.php Outdated Show resolved Hide resolved
app/Providers/PreferencesServiceProvider.php Outdated Show resolved Hide resolved
resources/views/glossary.blade.php Outdated Show resolved Hide resolved
resources/views/layouts/app.blade.php Show resolved Hide resolved
@mattstauffer
Copy link
Member Author

I would also suggest to try to keep the pr a little bit smaller so it is more easy to go through the changes that were made.

Yep.

@mattstauffer
Copy link
Member Author

Allowing a few todo's in because they relate to work addressed in #115

@mattstauffer mattstauffer merged commit 6058cea into master Nov 4, 2019
@mattstauffer mattstauffer deleted the pr/94 branch November 4, 2019 03:53
mattstauffer added a commit that referenced this pull request Nov 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Store the preferred way to show resources
3 participants