-
Notifications
You must be signed in to change notification settings - Fork 7
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
refactor skin selection #894
Changes from 20 commits
63880ce
47fac0e
3e10a42
5e12437
79bbd87
4710dad
88af7ec
a6a9fda
3077c2b
9e3d46b
4b51592
ee47952
2f4b06d
986354a
7fb22c4
9479d62
de526ea
2f96b13
d0a19fe
83a2216
69e5b70
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,15 +3,13 @@ | |
<v-card-title>Set Skin</v-card-title> | ||
<v-card-text> | ||
<v-select | ||
:items="items" | ||
:items="skins" | ||
label="Skin" | ||
placeholder="Pick a Skin" | ||
hint="The default skin is Vector." | ||
persistent-hint | ||
prepend-icon="mdi-web" | ||
v-model="skin" | ||
:disabled="inFlight" | ||
deer-wmde marked this conversation as resolved.
Show resolved
Hide resolved
|
||
:error-messages="error" | ||
v-model="skinId" | ||
></v-select> | ||
</v-card-text> | ||
<v-card-actions> | ||
|
@@ -22,45 +20,62 @@ | |
<span>It may take up to 10 seconds for changes to be reflected on your wiki</span> | ||
</v-tooltip> | ||
</v-card-actions> | ||
<Message ref="message" /> | ||
</v-card> | ||
</template> | ||
|
||
<script> | ||
import Message from '../Features/Message.vue' | ||
|
||
export default { | ||
name: 'Skin', | ||
components: { | ||
Message | ||
}, | ||
props: [ | ||
'wikiId' | ||
], | ||
data () { | ||
return { | ||
items: [ | ||
'Vector', | ||
'Modern', | ||
'Timeless' | ||
skins: [ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would expect There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah right, so we removed them again because they are not ready to be used yet. After our next mediawiki update we will be able to include them here. Until then I changed this PR so that it's just some code refactoring, basically preparation to add them in the future + reuse of the snackbar for alerts There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see I will then merge this |
||
{ | ||
value: 'vector', | ||
text: 'Vector' | ||
}, | ||
{ | ||
value: 'modern', | ||
text: 'Modern' | ||
}, | ||
{ | ||
value: 'timeless', | ||
text: 'Timeless' | ||
} | ||
], | ||
skin: '', | ||
inFlight: false, | ||
error: '' | ||
skinId: '', | ||
message: false | ||
} | ||
}, | ||
created () { | ||
const skin = this.$store.state.wikis.currentWikiSettings.wgDefaultSkin | ||
this.skin = skin.charAt(0).toUpperCase() + skin.slice(1) | ||
this.skinId = this.$store.state.wikis.currentWikiSettings.wgDefaultSkin | ||
}, | ||
computed: { | ||
skin () { | ||
return this.skins.find(skin => skin.value === this.skinId) | ||
} | ||
}, | ||
methods: { | ||
doSetSkin () { | ||
const wiki = this.wikiId | ||
// API needs the skin ID which is lower case.. | ||
const value = this.skin.toLowerCase() | ||
const value = this.skin.value | ||
|
||
this.$store | ||
.dispatch('updateSkin', { wiki, value }) | ||
.then(() => { | ||
alert('Update success!') | ||
this.$refs.message.show('success', `Your default skin has been updated to ${this.skin.text}.`) | ||
}) | ||
.catch(err => { | ||
console.log(err.response) | ||
alert('Something went wrong.') | ||
console.error(err.response) | ||
this.$refs.message.show('error', 'Something went wrong while updating your default skin. Please try again.') | ||
}) | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
<template> | ||
<v-snackbar :color="status" elevation="24" v-model="visible"> | ||
{{ text }} | ||
<template v-slot:action> | ||
<v-btn | ||
text | ||
variant="text" | ||
@click="hide" | ||
> | ||
Close | ||
</v-btn> | ||
</template> | ||
</v-snackbar> | ||
</template> | ||
|
||
<script> | ||
export default { | ||
data () { | ||
return { | ||
visible: false, | ||
text: 'Hello, this is a snackbar message!', | ||
status: 'success' | ||
} | ||
}, | ||
methods: { | ||
show (status, message) { | ||
this.status = status | ||
this.text = message | ||
this.visible = true | ||
}, | ||
|
||
hide () { | ||
this.visible = false | ||
} | ||
} | ||
} | ||
</script> | ||
|
||
<style scoped> | ||
</style> |
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.
https://v2.vuejs.org/v2/guide/components#Passing-Data-to-Child-Components-with-Props
I believe generally it's recommended to use props rather than
$refs
to pass data down to child components. Did you try this and find it wasn't possible?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.
nah I'm just a vue noob, will adjust soon
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.
just looked at the docs, I'm not just passing data but calling a method, not sure if there would be another way then
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 guess you could argue that we shouldnt know about the method and design the component in a way that changing the properties calls the method, but now I'm way out of bounds of my knowledge about vue architecture