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

Feat/622 Enable language selection before login #654

Merged
merged 8 commits into from
Oct 11, 2023
Merged

Feat/622 Enable language selection before login #654

merged 8 commits into from
Oct 11, 2023

Conversation

JonasThers
Copy link
Contributor

@JonasThers JonasThers commented Oct 9, 2023 •
edited
Loading

Resolves: #622

Screenshot 2023-10-09 at 22 46 54 Screenshot 2023-10-09 at 22 47 07 Screenshot 2023-10-09 at 23 02 47

I took the logic for language selection and put it in its own component, which is not nested in both TheUserMenu and LoginView

  • Do the tests still pass? (see Run the Tests)
  • Is the code formatted properly? (see Linting (Formatting))
  • For New Features:
    • Have tests been added to cover any new features or fixes?
    • Has the documentation been updated accordingly?

Please describe additional details for testing this change

seriouslysean
seriouslysean previously requested changes Oct 9, 2023
View reviewed changes
Copy link
Collaborator

@seriouslysean seriouslysean left a comment

Choose a reason for hiding this comment

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

The changes look sound but I have a question about implementation choice.

src/components/LanguageSelection.vue Outdated Show resolved Hide resolved
@JonasThers
Copy link
Contributor Author

Any idea how my changes in 2b3b7cf could cause this?
Screenshot 2023-10-11 at 0 00 33

@seriouslysean
Copy link
Collaborator

Any idea how my changes in 2b3b7cf could cause this?

This is a really annoying known issue with how sails is currently running alongside cypress. It's not from your PR.

The changes here look good and wouldn't cause that.

@itsalaidbacklife itsalaidbacklife added frontend Requires changes to the frontend (vue) client hacktoberfest This issue welcomes contributions for Hacktoberfest. labels Oct 11, 2023
itsalaidbacklife
itsalaidbacklife approved these changes Oct 11, 2023
View reviewed changes
Copy link
Contributor

@itsalaidbacklife itsalaidbacklife left a comment

Choose a reason for hiding this comment

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

Nicely done. This is a solid improvement to the UX and refactoring the language selector into its own component is a nice bonus for devEx. Thanks for your contribution!

@itsalaidbacklife itsalaidbacklife merged commit dce1ea2 into cuttle-cards:main Oct 11, 2023
@itsalaidbacklife itsalaidbacklife added the internationalization Translation of content into multiple languages label Nov 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@itsalaidbacklife itsalaidbacklife itsalaidbacklife approved these changes

@seriouslysean seriouslysean Awaiting requested review from seriouslysean

Assignees
No one assigned
Labels
frontend Requires changes to the frontend (vue) client hacktoberfest This issue welcomes contributions for Hacktoberfest. internationalization Translation of content into multiple languages
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feature]: Enable language selection from SignupView
3 participants
@JonasThers @seriouslysean @itsalaidbacklife