Skip to content
This repository has been archived by the owner on Mar 1, 2019. It is now read-only.

Add edit pen #137

Merged
merged 2 commits into from
Dec 21, 2018
Merged

Add edit pen #137

merged 2 commits into from
Dec 21, 2018

Conversation

jakobw
Copy link
Member

@jakobw jakobw commented Dec 20, 2018

@jakobw jakobw requested review from wiese and bitPogo and removed request for wiese December 20, 2018 17:23
@@ -9,6 +9,8 @@ import { ENTITY_INIT } from '@/store/entity/actionTypes';
import { LANGUAGE_PREFERENCE } from '@/store/user/actionTypes';
import { EDIT_LINK_URL_INIT } from '@/store/links/actionTypes';

config.logModifiedComponents = false;
Copy link
Member Author

@jakobw jakobw Dec 20, 2018

Choose a reason for hiding this comment

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

This has to do with vue-test-utils and vue-class-component. To be honest, I'm a little confused why this warning pops up in this file, which only shallowMounts components. Any ideas are welcome. Without this, we get the following warning:

console.error node_modules/@vue/test-utils/dist/vue-test-utils.js:15
[vue-test-utils]: The child component has been modified to ensure it is created with properties injected by Vue Test Utils.
This is because the component was created with Vue.extend, or uses the Vue Class Component decorator.
Because the component has been modified, it is not possible to find it with a component selector. To find the component, you must stub it manually using the stubs mounting option, or use a name or ref selector.
You can hide this warning by setting the Vue Test Utils config.logModifiedComponents option to false.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looked at it and #138 helped - please confirm.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ermahgerd, it does!!

wiese added a commit that referenced this pull request Dec 21, 2018
Update to latests @vue/test-utils.
This fixes, amongst others,
vuejs/vue-test-utils#973
encountered during #137
Copy link
Contributor

@wiese wiese left a comment

Choose a reason for hiding this comment

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

Swords to edit pens!

</script>

<style lang="scss">
.edit-action a {
Copy link
Contributor

Choose a reason for hiding this comment

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

This selector makes me uncomfortable as I'm afraid it will see matches from MW/WB.
Using a block identifier would work around that. It may feel repetitive but would give use piece of mind and make our components completely portable. Maybe it's the name that could be cooler to make this appear "more right"™, as in not bound to an implementation ("termbox") but rather part of the same set of building blocks in which we reap the fruits of well done isolation. Something completely generic like a project code name: hajamapa.

(webpack-contrib/css-loader#608 is something I found while pondering)

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice idea to push that into his own component!

Copy link
Member Author

Choose a reason for hiding this comment

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

hajamapa ಠ_ಠ

Copy link
Member Author

Choose a reason for hiding this comment

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

The generic class name is a good point. I added the .wikibase-termbox__ prefix, since it's correct for now.

@@ -9,6 +9,8 @@ import { ENTITY_INIT } from '@/store/entity/actionTypes';
import { LANGUAGE_PREFERENCE } from '@/store/user/actionTypes';
import { EDIT_LINK_URL_INIT } from '@/store/links/actionTypes';

config.logModifiedComponents = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Looked at it and #138 helped - please confirm.

jakobw pushed a commit that referenced this pull request Dec 21, 2018
Update to latests @vue/test-utils.
This fixes, amongst others,
vuejs/vue-test-utils#973
encountered during #137
Copy link
Member Author

@jakobw jakobw left a comment

Choose a reason for hiding this comment

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

Updated and rebased, to harness #138 goodness.

</script>

<style lang="scss">
.edit-action a {
Copy link
Member Author

Choose a reason for hiding this comment

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

hajamapa ಠ_ಠ

</script>

<style lang="scss">
.edit-action a {
Copy link
Member Author

Choose a reason for hiding this comment

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

The generic class name is a good point. I added the .wikibase-termbox__ prefix, since it's correct for now.

@wiese wiese merged commit f19e757 into master Dec 21, 2018
@wiese wiese deleted the add-pen branch December 21, 2018 11:08
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants