-
Notifications
You must be signed in to change notification settings - Fork 356
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
fix: Ensure column cell values update when columns are reordered #878
Conversation
553cf2b
to
5d545ac
Compare
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.
5d545ac
to
7931c79
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great @HeroicEric. I agree that it seems a massive simplification. If I can imagine a downside, it would be that there is a performance characteristic to the custom computed alias which is desirable.
That said I'm inclined to default toward correctness regardless unless there is a failure at the scale of "occlusion no longer works" or something.
Approving and will nudge for another maintainer to review as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's the only place that entire module is used? I'm in.
This is now released in |
Is there a reason to not bump the official release (or whatever is the name of the default version one gets from npm)? Currently it's |
@MichalBryxi I'll bump the minor after #901 lands. |
3.0.2 is out 🎉 |
For those who encounter this problem, our team encountered this very problem, and one of our team members discovered a fix in the form of using the value within |
We encountered this problem in the latest version (at the time of this writing, |
Fixes #808
Fixes #820
The idea behind this is basically the same as #567.
The value of
cellValue
is computed by grabbing thecolumnValue.valuePath
and then grabbing the value of the corresponding key on
rowValue
.Basically something like
rowValue.${columnValue.valuePath}
, which isnot possible using any of the built in computed macros.
This basically uses an observer to watch for changes to
columnValue.valuePath
, and defines/redefinescellValue
as a computedalias for the property at that path on
rowValue
. This ensures thecellValue
is correct ifcolumnValue.valuePath
or the actual value atthat path on
rowValue
changes. Observers aren't recommended but thiswas 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, itfeels OK to have a more simple implementation that is hard coded
specifically for this case.
Side note: I was also able to get this working by replacing the
CellWrapper
with a tracked version of the same class, which allowed me to avoid using any observers. I'd be interested in exploring some more to see if it would be feasible to conditionally swap out some of the pieces of this addon to use@tracked
if that's something the maintainers would be open to.