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

refactor skin selection #894

Merged
merged 21 commits into from
Oct 17, 2024
Merged

refactor skin selection #894

merged 21 commits into from
Oct 17, 2024

Conversation

deer-wmde
Copy link
Contributor

@deer-wmde deer-wmde commented Sep 24, 2024

https://phabricator.wikimedia.org/T375546

These changes allow the selection of Vector 2022 and MinervaNeue as default skins for a wiki.

  • I refactored some logic to separate the displayed strings from the values passed to the API, which allowed me to streamline the skin names how they appear in mediawiki itself

    • this turned out a little bit more complex than I hoped since the v-select component expects an array and not a json object
  • Additionally I refactored the simple alerts to the more pleasent looking snackbar alerts as we implemented them in the QuestCaptcha component

    • While I was at it I refactored that part into it's own component instead of copy-pasting the code

Copy link

Deployment previews on netlify for branch refs/pull/894/merge will be at the following locations (when build is done):

</v-btn>
</template>
</v-snackbar>
<Message ref="message" />
Copy link
Contributor

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?

Copy link
Contributor Author

@deer-wmde deer-wmde Sep 25, 2024

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

Copy link
Contributor Author

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

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 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

Copy link
Contributor

@rosalieper rosalieper left a comment

Choose a reason for hiding this comment

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

I tried it locally and did not see more skins

'Vector',
'Modern',
'Timeless'
skins: [
Copy link
Contributor

Choose a reason for hiding this comment

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

I would expect minerva and vector-2022 to be part of this array. As I understood we want them added to the skins

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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

Copy link
Contributor

Choose a reason for hiding this comment

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

I see I will then merge this

@rosalieper rosalieper merged commit 2d09c5a into main Oct 17, 2024
7 checks passed
@rosalieper rosalieper deleted the de/new-skins branch October 17, 2024 12:10
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