Skip to content
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

Proxyable row options #194

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

billdami
Copy link

An attempt at solving the problem discussed in #151, allowing row options/state properties (notably, the selected state) to be proxied to the row's content object. However, this PR feels kinda messy/ugly and probably is not the best implementation, so its more intended as a way to kick off further discussion and ideas.

A major thing to point out is that I ended up requiring a global option, proxyRowOptions to be enabled in order to opt-in to the proxying of the row options. I did this because I think without it, this would be a potentially breaking change to the API, since setting the row's hidden, expanded, and selected properties are now updating the row's content object itself, which may already contain properties with the same name that are used for other purposes.

@offirgolan
Copy link
Collaborator

@billdami thank you for submitting this PR! Although I understand the use-case for this, I dont believe this is the correct solution. In all honesty, with the current implementation, I dont think there is a good solution for this. This could be one of those things that you will have to go around until we can figure out a decent solution.

@taras I would love your opinion on this one.

@billdami
Copy link
Author

billdami commented Oct 5, 2016

@offirgolan No problem! Yeah it definitely didn't feel like the best solution as I was implementing it, but as I mentioned in the OP, I more intended it to be a jumping off point for some more serious discussion on how it should be done. :)

@offirgolan
Copy link
Collaborator

@billdami I'm glad you feel the same way! I totally agree with you on the fact that this needs some much needed discussion. As of right now, there are some bigger features that we are trying to push for so we will have to place this issue on the backlog until we have some more time. Thanks again for taking the time to put this together! 😸

@RobbieTheWagner
Copy link
Member

@billdami Thanks for the PR! We've recently made a lot of significant changes to ember-light-table. Can you please rebase your PR and make sure everything is still working as intended? If this PR is no longer necessary after the new changes, please close it. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants