Skip to content
This repository was archived by the owner on Mar 25, 2021. It is now read-only.

Dispatch Input based in property datatype#123

Merged
micgro42 merged 1 commit into
masterfrom
ValueInputDispatcher
Jan 13, 2021
Merged

Dispatch Input based in property datatype#123
micgro42 merged 1 commit into
masterfrom
ValueInputDispatcher

Conversation

@micgro42
Copy link
Copy Markdown
Collaborator

@micgro42 micgro42 commented Jan 4, 2021

(To be merged into master after #142)

Todo:

  • agree on the basic approach
  • tests

Bug: T270257

@micgro42 micgro42 requested review from Ladsgroup and guergana January 4, 2021 16:49
@micgro42 micgro42 force-pushed the ValueInputDispatcher branch 2 times, most recently from cd84139 to c143726 Compare January 5, 2021 08:59
@micgro42 micgro42 marked this pull request as ready for review January 5, 2021 09:00
@micgro42 micgro42 marked this pull request as draft January 5, 2021 13:39
@micgro42 micgro42 force-pushed the ValueInputDispatcher branch 3 times, most recently from 55d5506 to 5a85c5b Compare January 6, 2021 12:22
@micgro42 micgro42 marked this pull request as ready for review January 6, 2021 12:26
@micgro42 micgro42 changed the base branch from master to explicitlyUnsetProperty January 6, 2021 12:26
@micgro42 micgro42 force-pushed the ValueInputDispatcher branch 2 times, most recently from c6d8d31 to 367c79a Compare January 6, 2021 12:41
@micgro42 micgro42 force-pushed the explicitlyUnsetProperty branch from 41863fc to 8cdfb10 Compare January 7, 2021 15:47
@micgro42 micgro42 force-pushed the ValueInputDispatcher branch 2 times, most recently from d2abab6 to 2331149 Compare January 7, 2021 15:57
@micgro42 micgro42 requested a review from jakobw January 7, 2021 20:26
Copy link
Copy Markdown
Member

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

Looks good! I tried it out locally and it works nicely! 🏆

I left a suggestion in there for the value input dispatching code. I don't think this necessarily has to happen in this PR but I'm curious to hear thoughts.

Comment thread src/components/ValueInput.vue Outdated
Comment on lines +2 to +8
<ItemValueLookup
v-if="datatype === 'wikibase-item'"
:value="value"
@input="$emit('input', $event)"
:error="error"
:disabled="disabled"
/>
<TextInput
v-else
:value="value"
:label="$i18n('query-builder-input-value-label')"
@input="$emit('input', $event)"
:error="error && { message: $i18n(error.message), type: error.type }"
:placeholder="$i18n('query-builder-input-value-placeholder')"
:disabled="disabled"
/>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If value input components share the same interface (which they probably should) this whole template (or the whole component?) could turn into something like this:

Suggested change
<ItemValueLookup
v-if="datatype === 'wikibase-item'"
:value="value"
@input="$emit('input', $event)"
:error="error"
:disabled="disabled"
/>
<TextInput
v-else
:value="value"
:label="$i18n('query-builder-input-value-label')"
@input="$emit('input', $event)"
:error="error && { message: $i18n(error.message), type: error.type }"
:placeholder="$i18n('query-builder-input-value-placeholder')"
:disabled="disabled"
/>
<component
:is="{
string: 'TextInput',
'external-id': 'TextInput',
'wikibase-item': 'ItemValueLookup'
}[datatype]"
:value="value"
:label="$i18n('query-builder-input-value-label')"
@input="$emit('input', $event)"
:error="error && { message: $i18n(error.message), type: error.type }"
:placeholder="$i18n('query-builder-input-value-placeholder')"
:disabled="disabled"
/>

... but ideally without doing the dispatching in the template.

If that works out, all this component does is forwarding props. At that point it seems cleaner to put the dispatching code into a regular function, and move the differences we see in the templates (error handling, messages, ...) down into the value input components. Then this can become a functional component or be removed entirely if nothing is left.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I really like the looks of this, thanks! I had looked at <component is="" previously and it didn't seem exactly right, but your approach is very convincing.

Then the first step for this would be to wrap the TextInput into its own component to align the interfaces, right?. That was on my Todo-list anyway, I will look into it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Then the first step for this would be to wrap the TextInput into its own component to align the interfaces, right?

Yes, that sounds good!

@micgro42 micgro42 marked this pull request as draft January 8, 2021 12:15
@micgro42 micgro42 force-pushed the explicitlyUnsetProperty branch from 8cdfb10 to 5bb9de5 Compare January 8, 2021 15:56
@micgro42 micgro42 force-pushed the ValueInputDispatcher branch from 2331149 to 04195e2 Compare January 8, 2021 16:05
@micgro42 micgro42 changed the base branch from explicitlyUnsetProperty to addMissingProps January 8, 2021 16:06
@micgro42 micgro42 mentioned this pull request Jan 8, 2021
@micgro42 micgro42 force-pushed the ValueInputDispatcher branch from 04195e2 to f1a3c0c Compare January 8, 2021 16:08
@micgro42 micgro42 marked this pull request as ready for review January 8, 2021 16:11
Copy link
Copy Markdown
Member

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

I think this looks super good! ValueInput.vue could use a test though, and it should be very nicely testable now.

@micgro42 micgro42 force-pushed the ValueInputDispatcher branch from f1a3c0c to 1090647 Compare January 12, 2021 14:07
@micgro42 micgro42 force-pushed the ValueInputDispatcher branch from 1090647 to e58f9a4 Compare January 12, 2021 14:20
@micgro42 micgro42 requested a review from jakobw January 12, 2021 14:20
Copy link
Copy Markdown
Member

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

Woo! 🔥

Base automatically changed from addMissingProps to master January 13, 2021 11:18
@micgro42 micgro42 force-pushed the ValueInputDispatcher branch from e58f9a4 to cd5b138 Compare January 13, 2021 11:19
@micgro42 micgro42 merged commit 2e7c316 into master Jan 13, 2021
@micgro42 micgro42 deleted the ValueInputDispatcher branch January 13, 2021 11:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants