This repository was archived by the owner on Mar 25, 2021. It is now read-only.
Extract EntityLookup and add ItemValueLookup#111
Merged
Conversation
Contributor
|
I'm not 100% we should do this. The amount of duplicated code is small and if it grows, it should be moved the pure ts/js instead of being in Vue. Some of the code you put here can be moved to pure ts/js and reuse that in the components. |
micgro42
force-pushed
the
ItemValueLookup
branch
from
48092ac to
6bd604a
Compare
micgro42
force-pushed
the
ItemValueLookup
branch
from
6bd604a to
f6f366f
Compare
Collaborator
Author
|
Rebased it on top of master |
Collaborator
Author
Yeah, at some point we might want to share code with a "Unit Lookup" 🤔 |
micgro42
force-pushed
the
ItemValueLookup
branch
2 times, most recently
from
4ff4365 to
394fcc7
Compare
ItemValueLookup and PropertyLookup share a lot of logic, yet there are subtle differences, both in logic and in semantics. This is passing down a method as a prop. While that might be more unusual in vuejs than in react, it seems that having the parent maintain the state only for it to be passed down to the child and having the child ask for more items on scroll would seem to be much more convoluted and clumsy. Bug: T270256
micgro42
force-pushed
the
ItemValueLookup
branch
from
394fcc7 to
9fc3306
Compare
Ladsgroup
reviewed
Jan 4, 2021
| }, | ||
| }, | ||
| props: { | ||
| searchForMenuItems: { |
Contributor
There was a problem hiding this comment.
Passing it as a prop. Nice idea!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ItemValueLookup and PropertyLookup share a lot of logic, yet there are
subtle differences, both in logic and in semantics.
This is passing down a method as a prop. While that might be more
unusual in vuejs than in react, it seems that having the parent maintain
the state only for it to be passed down to the child and having the
child ask for more items on scroll would seem to be much more convoluted
and clumsy.
TODO:
Bug: T270256