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

BZ#1501098 Fix for Service UI not taking default language #1375

Merged
merged 6 commits into from
Feb 2, 2018
Merged

BZ#1501098 Fix for Service UI not taking default language #1375

merged 6 commits into from
Feb 2, 2018

Conversation

chalettu
Copy link
Contributor

@chalettu chalettu commented Jan 29, 2018 •
edited by simaishi
Loading

@miq-bot add_label bug
@miq-bot add_label gaprindashvili/yes

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1501098

@chalettu
Updated how language selection works
e8beec6 
Also migrated away from using a language selector directive to a component
@chalettu
Fixed linting issues
1a2cf58
@chalettu
Copy link
Contributor Author

This PR is accomplishing a handful of things.

  1. Change the order of precedence for how language is set and persisted in service UI.
  2. Conversion of the language-switcher directive into an angular component
  3. Made sure the language selections made on the login page of Service UI get persisted and are honored.

Here is how the language selection order of precedence works.
If in Classic UI a language is set as default and that is the only setting made, then SUI will assume this language.
If in Classic UI a user changes their default language selection that language will be used by SUI.
If you choose to use the language switcher on the Service UI login page or on the nav menu switcher, those changes will change your language settings for both Service UI and Classic UI.

@chalettu
Copy link
Contributor Author

So this PR contains some CSS changes but they are all made in support of converting the directive into an angular component. Still, I decided to do a quick SS of the changed code. No UX changes should be required for this PR
language-selector

@AllenBW AllenBW self-assigned this Jan 30, 2018
@AllenBW
Copy link
Member

AllenBW commented Jan 30, 2018

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1501098

AllenBW
AllenBW suggested changes Jan 30, 2018
View reviewed changes
Copy link
Member

@AllenBW AllenBW left a comment

Choose a reason for hiding this comment

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

Minor feedback, overall looks great, any chance we can get a gif of it in action?

client/app/layouts/_application.sass Outdated
@@ -85,3 +86,22 @@ li
height: 38px
margin-top: 10px
width: 320px

.language-switcher
Copy link
Member

Choose a reason for hiding this comment

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

Waddya think about poppin' this sass in _language-switcher.sass?

Copy link
Contributor Author

@chalettu chalettu Jan 30, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Was able after a bit of work to get the css under language-switcher.sass

client/app/core/core.module.js Outdated
@@ -22,7 +22,7 @@ import { DialogFieldRefreshFactory } from './dialog-field-refresh.service.js'
import { EventNotificationsFactory } from './event-notifications.service.js'
import { ExceptionModule } from './exception/exception.module.js'
import { LanguageFactory } from './language.service.js'
import { LanguageSwitcherDirective } from './language-switcher/language-switcher.directive.js'
import { LanguageSwitcherComponent } from './language-switcher/language-switcher.component.js'
Copy link
Member

Choose a reason for hiding this comment

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

Thoughts on moving this to ./app/components?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought we were going to get rid of that folder, how about /app/shared ?

Copy link
Member

Choose a reason for hiding this comment

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

OOoooo Shared kinda makes sense... it is after all used in two distinct places, login.html and application.html (one is a state the latter a layout)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved the component to /app/shared

@chalettu
Moved Language switcher to shared
66650b2 
Also changed where css is declared for the component
@chalettu
Copy link
Contributor Author

chalettu commented Jan 30, 2018 •
edited
Loading

Demos of language selection on login page
language-selector-login-page

@chalettu
Copy link
Contributor Author

Demo of changing language while logged in
language-selector-logged-in

@chalettu chalettu changed the title BZ#1501098 Fix for Service UI not taking default language [WIP] BZ#1501098 Fix for Service UI not taking default language Jan 30, 2018
@miq-bot miq-bot added the wip label Jan 30, 2018
@chalettu chalettu changed the title [WIP] BZ#1501098 Fix for Service UI not taking default language BZ#1501098 Fix for Service UI not taking default language Jan 30, 2018
@miq-bot miq-bot removed the wip label Jan 30, 2018
AllenBW
AllenBW suggested changes Feb 1, 2018
View reviewed changes
Copy link
Member

@AllenBW AllenBW left a comment

Choose a reason for hiding this comment

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

lang

If i login with a set language it works! Super awesome
If i reload the page from the dashboard language is still set! Super awesome
If i navigate to another state and reload the page, lnav looks to lose the set language 😭

@miq-bot
Copy link
Member

miq-bot commented Feb 1, 2018

Checked commits https://github.com/chalettu/manageiq-ui-service/compare/e8beec6ea6e9c679a384741b688d4c2258ba2376~...932b4e2b4c7ac99ab9381a94e32b36ad70d9d6d5 with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
0 files checked, 0 offenses detected
Everything looks fine. 🍰

@chalettu
Copy link
Contributor Author

chalettu commented Feb 1, 2018

change_language_reload
Screenshot of fix for navigating to another page after changing language and hitting reload page. @AllenBW take a look. Good find by the way.

AllenBW
AllenBW approved these changes Feb 2, 2018
View reviewed changes
Copy link
Member

@AllenBW AllenBW left a comment

Choose a reason for hiding this comment

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

Looks good @chalettu thanks for the quick turn around on feedback <3

@AllenBW AllenBW merged commit 4648996 into ManageIQ:master Feb 2, 2018
@simaishi simaishi mentioned this pull request Feb 5, 2018
Merged
4 tasks
@simaishi
Copy link
Contributor

simaishi commented Mar 8, 2018

@chalettu Changes in client/app/core/navigation/navigation-controller.js is conflicting on backport because similar change was made in gaprindashvili PR #1378. I'm not sure what to take, please clarify.

@himdel
Copy link
Contributor

himdel commented Feb 5, 2019

Second BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1672324

Will take care of the conflict...

@himdel himdel mentioned this pull request Feb 11, 2019
Closed
@himdel
Copy link
Contributor

himdel commented Feb 11, 2019

Created #1526 for gaprindashvili

@AllenBW AllenBW modified the milestone: Sprint 105 Ending Feb 18, 2019 Feb 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AllenBW AllenBW AllenBW approved these changes

@himdel himdel Awaiting requested review from himdel

Assignees

@AllenBW AllenBW

Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@chalettu @AllenBW @miq-bot @simaishi @himdel