[bugfix] Properly alias cellValue #567
Merged
Conversation
This file contains 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
cellValue is a dynamically computed value that depends on the row value and a dynamic key (the `column.valuePath` property). Currently it updates the value once, and never after that. It can't respond to external changes, and changes can't be bound. This introduces a computed property that aliases based on a dynamic key. It's based on the work in ember-macro-helpers, simplified a bit since we don't need all the functionality in there. It could possibly be simplified further in the future, but should get us where we need to be for now.
HeroicEric
pushed a commit
to HeroicEric/ember-table
that referenced
this pull request
Mar 9, 2021
Fixes Addepar#808 Fixes Addepar#820 The idea behind this is basically the same as Addepar#567. The value of `cellValue` is computed by grabbing the `columnValue.valuePath` and then grabbing the value of the corresponding key on `rowValue`. Basically something like `rowValue.${columnValue.valuePath}`, which is not possible using any of the built in computed macros. This basically uses an observer to watch for changes to `columnValue.valuePath`, and defines/redefines `cellValue` as a computed alias for the property at that path on `rowValue`. This ensures the `cellValue` is correct if `columnValue.valuePath` or the actual value at that path on `rowValue` changes. Observers aren't recommended but this was already using one. Previously this was solved by creating a more generic `dynamicAlias` computed macro in Addepar#567. To be honest, I was having a little trouble wrapping my head around all that was happening in there but I think the changes in this PR accomplish the same idea. I'm not totally sure what the issue was with the other implementation but, since the `dynamicAlias` macro was only used in this one place, it feels OK to have a more simple implementation that is hard coded specifically for this case.
HeroicEric
added a commit
to HeroicEric/ember-table
that referenced
this pull request
Mar 9, 2021
Fixes Addepar#808 Fixes Addepar#820 The idea behind this is basically the same as Addepar#567. The value of `cellValue` is computed by grabbing the `columnValue.valuePath` and then grabbing the value of the corresponding key on `rowValue`. Basically something like `rowValue.${columnValue.valuePath}`, which is not possible using any of the built in computed macros. This basically uses an observer to watch for changes to `columnValue.valuePath`, and defines/redefines `cellValue` as a computed alias for the property at that path on `rowValue`. This ensures the `cellValue` is correct if `columnValue.valuePath` or the actual value at that path on `rowValue` changes. Observers aren't recommended but this was already using one. Previously this was solved by creating a more generic `dynamicAlias` computed macro in Addepar#567. To be honest, I was having a little trouble wrapping my head around all that was happening in there but I think the changes in this PR accomplish the same idea. I'm not totally sure what the issue was with the other implementation but, since the `dynamicAlias` macro was only used in this one place, it feels OK to have a more simple implementation that is hard coded specifically for this case.
HeroicEric
added a commit
to HeroicEric/ember-table
that referenced
this pull request
Mar 9, 2021
Fixes Addepar#808 Fixes Addepar#820 The idea behind this is basically the same as Addepar#567. The value of `cellValue` is computed by grabbing the `columnValue.valuePath` and then grabbing the value of the corresponding key on `rowValue`. Basically something like `rowValue.${columnValue.valuePath}`, which is not possible using any of the built in computed macros. This basically uses an observer to watch for changes to `columnValue.valuePath`, and defines/redefines `cellValue` as a computed alias for the property at that path on `rowValue`. This ensures the `cellValue` is correct if `columnValue.valuePath` or the actual value at that path on `rowValue` changes. Observers aren't recommended but this was already using one. Previously this was solved by creating a more generic `dynamicAlias` computed macro in Addepar#567. To be honest, I was having a little trouble wrapping my head around all that was happening in there but I think the changes in this PR accomplish the same idea. I'm not totally sure what the issue was with the other implementation but, since the `dynamicAlias` macro was only used in this one place, it feels OK to have a more simple implementation that is hard coded specifically for this case.
mixonic
pushed a commit
that referenced
this pull request
Mar 17, 2021
Fixes #808 Fixes #820 The idea behind this is basically the same as #567. The value of `cellValue` is computed by grabbing the `columnValue.valuePath` and then grabbing the value of the corresponding key on `rowValue`. Basically something like `rowValue.${columnValue.valuePath}`, which is not possible using any of the built in computed macros. This basically uses an observer to watch for changes to `columnValue.valuePath`, and defines/redefines `cellValue` as a computed alias for the property at that path on `rowValue`. This ensures the `cellValue` is correct if `columnValue.valuePath` or the actual value at that path on `rowValue` changes. Observers aren't recommended but this was already using one. Previously this was solved by creating a more generic `dynamicAlias` computed macro in #567. To be honest, I was having a little trouble wrapping my head around all that was happening in there but I think the changes in this PR accomplish the same idea. I'm not totally sure what the issue was with the other implementation but, since the `dynamicAlias` macro was only used in this one place, it feels OK to have a more simple implementation that is hard coded specifically for this case. Co-authored-by: Eric Kelly <HeroicEric@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
cellValue is a dynamically computed value that depends on the row value
and a dynamic key (the
column.valuePathproperty). Currently itupdates the value once, and never after that. It can't respond to
external changes, and changes can't be bound.
This introduces a computed property that aliases based on a dynamic key.
It's based on the work in ember-macro-helpers, simplified a bit since we
don't need all the functionality in there. It could possibly be
simplified further in the future, but should get us where we need to be
for now.