Skip to content

Conversation

@jcperez-ch
Copy link
Contributor

As mentioned in #161 and #160

The spec:

When syncFilters is true and a unfiltered action is dispatched: The _latestUnfiltered state will change with the value of the present state
When syncFilters is false and a unfiltered action is dispatched: The _latestUnfiltered state will remain as before.
When a filtered action is dispatched: The syncFilters flag is irrelevant.

@jcperez-ch
Copy link
Contributor Author

@davidroeca here it is the syncFilters hack/feature. Please let me know if you are OK with it.

@davidroeca
Copy link
Collaborator

@jcperez-ch thanks for the PR, looks good to me! @omnidan, thoughts? Might want to think about version numbers

@omnidan
Copy link
Owner

omnidan commented Jun 1, 2017

I still feel like there must be a better solution to all these issues than adding more and more flags to the library. But for now, I guess it's the best we can come up with. I will merge this and release beta9-9-1 (unfortunately, I cannot make a beta10 release because npm thinks it's lower than beta9)

@omnidan omnidan merged commit d78c753 into omnidan:master Jun 1, 2017
@omnidan
Copy link
Owner

omnidan commented Jun 1, 2017

I released beta9-9-1, let me know if this feature solves your issue @jcperez-ch !

@davidroeca
Copy link
Collaborator

@omnidan agreed, I think the challenge I've seen with all of this is the question of what we can consider stable and how updates to functionality should proceed. A lot of people have differing opinions on how certain features should be implemented--the most contentious being filtering, since the behavior has evolved pretty drastically.

Noting these beta version numbers and how the winds have shifted, I think a full-scale refactoring may be in order. This may cause breaking changes to some these features while at the same time keeping the rationale behind each of these flags in mind. Potentially have a release candidate (rc or candidate) versioning scheme, with set mile stones and a focus on a cleaner design with a maybe a little more abstraction?

@davidroeca
Copy link
Collaborator

Might need to refactor the tests as well, and figure out how those can be more easily grown

@omnidan
Copy link
Owner

omnidan commented Jun 1, 2017

@davidroeca I agree, a refactor and rc release is a good idea. I think most complex filter behaviour could be implemented by allowing users to define a certain kind of filter function (instead of having booleans as tags). Documentation also really needs to be reworked before release. We could put commonly used filter functions there (and other frequently asked questions).

Feel free to mention or PM me on gitter if you want to discuss this more 😁

@jcperez-ch
Copy link
Contributor Author

Ok guys, yes I know, it feels kinda wrong to have a flag that covers a use case of filter. This will solve the use case I am having, but, now I am planing to rethink the behaviour.

@davidroeca
Copy link
Collaborator

It seems the use case here is to redo to a filtered state. So it could just add _latestUnfiltered and present to the future (if different) on a single undo. But that's a little counter-intuitive with sizes.

The trade-off is that any way you slice it, you'll have to undo/redo too many times if you don't want your filtered actions to affect the undo/redo steps. Maybe I'm in the minority for my preference here.

It's nice to have both but potentially more bug prone. I don't see a way to have filter functions that would solve for these differences, since it's a question of how already-filtered states are managed by the library

@davidroeca
Copy link
Collaborator

Thinking more about this, I'm in favor of pushing both on undo, and having to redo twice to get to a filtered state. This gives the user the ability to get back to they were when they clicked undo, while at the same time keeping unfiltered steps the top priority

@jcperez-ch jcperez-ch deleted the syncFilter branch June 2, 2017 14:45
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.

3 participants