-
Notifications
You must be signed in to change notification settings - Fork 156
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
[full-ci] Vue 3: Use vue3-gettext
#8257
Conversation
19e2c76
to
323475d
Compare
Results for acceptance oC10 https://drone.owncloud.com/owncloud/web/31766/13/1 |
323475d
to
0b2ef85
Compare
0b2ef85
to
a97d466
Compare
import translations from '../l10n/translations.json' | ||
|
||
const config = { | ||
language: 'en', // TODO BEFORE MERGE: Access current locale |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that we removed this TODO BEFORE MERGE from master 😂
a97d466
to
63629b3
Compare
79c31cd
to
17550ac
Compare
e8252f5
to
656628e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only found a few things that are absolutely optional, just more to my personal taste :)
good work 🎸 !
@@ -24,6 +24,9 @@ describe('OcStatusIndicators', () => { | |||
resource: fileResource, | |||
indicators: [indicator], | |||
target: 'test' | |||
}, | |||
global: { | |||
plugins: [...defaultPlugins()] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why spreading? unnecessary [Symbol.iterator]
call.
defaultPlugins()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically all tests have it this way, just because it is easier to add more plugins. We think the overhead can be neglect in the test suite.
@@ -35,6 +38,9 @@ describe('OcStatusIndicators', () => { | |||
resource: fileResource, | |||
indicators: [indicator], | |||
target: 'test' | |||
}, | |||
global: { | |||
plugins: [...defaultPlugins()] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
@@ -128,6 +128,7 @@ describe('OcTable.sort', () => { | |||
data | |||
}, | |||
global: { | |||
plugins: [...defaultPlugins()], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
@@ -175,6 +176,7 @@ describe('OcTable.sort', () => { | |||
sortDir: sortDirOld | |||
}, | |||
global: { | |||
plugins: [...defaultPlugins()], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same ...
store: Store<any> | ||
}): Promise<void> => { | ||
}): Promise<any> => { | ||
const store = new Store({ ...storeOptions }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if storeOptions are deep i would prefer to use Object.assign (shallow clone)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...
is shallow as well AFAIK. We just moved this part basically, it was like this before as well.
createGettext({ | ||
defaultLanguage: navigator.language.substring(0, 2), | ||
silent: true, | ||
...options |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you put the options spreading upfront? My personal taste :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case we want options to possibly overwrite the defaults, so we need it in this order. The other cases above are valid though, I adjusted it 👍
Kudos, SonarCloud Quality Gate passed! |
Description
Use
vue3-gettext
and get rid of the old version.We also removed the store from the
defaults.js
file as we now handle this inannounceStore()
. This was not necessary for this PR, but since we touched that code anyways we thought it would be nicer.#7948
Types of changes