organice
organice copied to clipboard
200ok-ch
Fix "Visibile" typo
This will presumably drop any existing value for this setting (in the store and/or .organice-config.json), so I'm leaving it as a draft for now. I'd need help on how to handle a safe migration from the typo to the correctly spelt version.
Good catch, that the config option is persisted in the config file!
In my opinion, Organice – as a rolling release – could just change this option.
- a migration policy would add complexity and code that should be deleted in a few weeks or months.
- a silent config change of this option is not too bad: after a few conflict resolution dialogs from Organice, users probably fix it in their options ;)
What do you think?
BTW: interesting, how do you mark a PR as draft?
@schoettl commented on June 2, 2020 9:27 PM:
Good catch, that the config option is persisted in the config file!
In my opinion, Organice – as a rolling release – could just change this option.
- a migration policy would add complexity and code that should be deleted in a few weeks or months.
- a silent config change of this option is not too bad: after a few conflict resolution dialogs from Organice, users probably fix it in their options ;)
What do you think?
I agree.
BTW: interesting, how do you mark a PR as draft?
You just click the button ;-)
@munen Do you agree with @schoettl's assessment here? I think I do, so marking ready for review.
Thanks for the PR, fixing typos certainly helps the codebase to become more readable. I apologize for putting it in there in the first place!
If possible, I'd like to not make breaking changes for existing users. I added this feature in the first place so that people don't lose work or data. Without this, it's very easy for people to get into a conflict state. Having build some products, my experience is not that people will silently go to the settings and re-enable something that they previously enabled and be fine with it - they will rightfully be very unhappy and might even opt to not use organice, again, because it broke their workflow. Reducing churn is a great way to increase the user and contributor base.
organice actually does have migrations for making changes like this one! And if written in an idempotent manner, there's no need to remove them (similarly to DB migrations in back-end frameworks). I've added such a migration to the PR: https://github.com/200ok-ch/organice/pull/319/commits/3776af07a55d769b069e45ec811480b99bc3eba9
Unfortunately, introducing this has the same issue as #320. After running the code, the setting is gone:
localStorage.getItem("shouldSyncOnBecomingVisible")
"undefined"
The settings implementation currently is not very solid (based on the two problems in two PRs). Since we're about to potentially break two existing settings, now is the best time to clean that up. I'm very happy to help and already invested quite some time on #320 and will continue to do so when I can!
OK great - agreed on all points. Maybe together we can figure out what's going on here!
I made a screencast where you can see that the current configuration shows in the settings page and in the JSON. Then, when switching to the new branch (with the typo fixed), the setting doesn't show anymore and is removed without replacement in the config JSON.
Adding a blocked label to show that we want to resolve the default settings issue, first.