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

Pass "?locale" URL query param to oVirt API #131

Merged
merged 1 commit into from
Nov 28, 2017

Conversation

mareklibra
Copy link
Contributor

@mareklibra mareklibra commented Apr 10, 2017

With this change, the oVirt API returns localized error messages, so
they can be displayed to the user without additional modification.

Fixes: #110


This change is Reviewable

@mareklibra
Copy link
Contributor Author

@jelkosz , please try.
Works for me :-)

@mareklibra mareklibra added the Status: Blocked blocked by something. describe what is blocking it label Apr 12, 2017
@mareklibra
Copy link
Contributor Author

Blocked by missing translations in oVirt API.
Or until different approach is found.

@jelkosz
Copy link
Contributor

jelkosz commented Nov 2, 2017

@mareklibra it is not blocked anymore, we can normally use the ?locale=en_US or whatever now and it will work.

@mareklibra mareklibra removed the Status: Blocked blocked by something. describe what is blocking it label Nov 3, 2017
@mareklibra
Copy link
Contributor Author

Needs rebase

@jniederm
Copy link
Contributor

jniederm commented Nov 6, 2017

Review status: 0 of 3 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.


src/config.js, line 18 at r1 (raw file):

function parseQueryParams () {
  AppConfiguration.queryParams.locale = getURLQueryParameterByName('locale')

Maybe we can unify locale resolution with intl module:

import intl from 'intl'
AppConfiguration.queryParams.locale = intl.locale

or use intl module directly in ovirtapi.js


src/helpers.js, line 98 at r1 (raw file):

}

export function getURLQueryParameterByName (name, url) {

Is there any need for url argument? I can't it to be used.

How about getUrlQueryParameterByName instead of getURLQueryParameterByName?


src/helpers.js, line 100 at r1 (raw file):

export function getURLQueryParameterByName (name, url) {
  if (!url) {
    url = window.location.href

for http://example.com/page?hello=world&foo=bar window.location.search returns ?hello=world&foo=bar. Maybe search property can be used.


Comments from Reviewable

src/helpers.js Outdated
return ''
}

return decodeURIComponent(results[2].replace(/\+/g, ' '))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do logging of result here

@mareklibra mareklibra force-pushed the issue-110.localeQueryParamToBackend branch from c353c3a to 7560ac3 Compare November 28, 2017 09:04
@mareklibra
Copy link
Contributor Author

Rebased

With this change, the oVirt API returns localized error messages, so
they can be displayed to the user without additional modification.

Fixes: #110
@mareklibra mareklibra force-pushed the issue-110.localeQueryParamToBackend branch from 7560ac3 to 7795c48 Compare November 28, 2017 09:14
@mareklibra
Copy link
Contributor Author

Review status: 0 of 3 files reviewed at latest revision, 4 unresolved discussions.


src/config.js, line 18 at r1 (raw file):

Previously, jniederm wrote…

Maybe we can unify locale resolution with intl module:

import intl from 'intl'
AppConfiguration.queryParams.locale = intl.locale

or use intl module directly in ovirtapi.js

good point, but let's postpone that for a follow-up due to time constraints. Must be properly verified against engine side due to potential differences (i.e. '-' vs '_' or different browser locale)


src/helpers.js, line 98 at r1 (raw file):

Previously, jniederm wrote…

Is there any need for url argument? I can't it to be used.

How about getUrlQueryParameterByName instead of getURLQueryParameterByName?

Not anymore, removed


src/helpers.js, line 100 at r1 (raw file):

Previously, jniederm wrote…

for http://example.com/page?hello=world&foo=bar window.location.search returns ?hello=world&foo=bar. Maybe search property can be used.

Good point, but this works as well.


src/helpers.js, line 113 at r1 (raw file):

Previously, mareklibra (Marek Libra) wrote…

Do logging of result here

Done.


Comments from Reviewable

@jniederm
Copy link
Contributor

Reviewed 3 of 3 files at r2.
Review status: all files reviewed at latest revision, 4 unresolved discussions.


src/config.js, line 18 at r1 (raw file):

Previously, mareklibra (Marek Libra) wrote…

good point, but let's postpone that for a follow-up due to time constraints. Must be properly verified against engine side due to potential differences (i.e. '-' vs '_' or different browser locale)

based on offline communication: consumers are different: web-ui localization vs Accept-Language header for REST API.


Comments from Reviewable

@mareklibra mareklibra merged commit 74057e7 into master Nov 28, 2017
@mareklibra mareklibra deleted the issue-110.localeQueryParamToBackend branch July 27, 2018 06:27
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.

3 participants