-
Notifications
You must be signed in to change notification settings - Fork 74
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
Adding a Comma and Period option to the settings menu #340
Adding a Comma and Period option to the settings menu #340
Conversation
Thank you for the pull request!The Scribe team will do our best to address your contribution as soon as we can. The following is a checklist for maintainers to make sure this process goes as well as possible. Feel free to address the points below yourself in further commits if you realize that actions are needed :) If you're not already a member of our public Matrix community, please consider joining! It'd be great to have you! Maintainer checklist
|
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.
Changes look good to me, @SaurabhJamadagni 😊 Obviously the hacky behavior of jumping to Installation
from Settings
on reload is not ideal, but let's just be sure to have this fixed in the coming weeks when we get ready to release v3.0.0
. I can also try to look into this when I re-implement the German app localization :)
@andrewtavis, could you open up an issue for it? I think it would be good to document the changes that follow in case we come across a bug similar to this down the line. Also, should serve as a reminder for us as well. Or I'll create the issue tomorrow if you are occupied. No worries 😄 |
I’ll make the issue now, @SaurabhJamadagni :) |
See #344, @SaurabhJamadagni 😊 |
Contributor checklist
Description
Hey @andrewtavis, here's one more giant PR again unfortunately 🙈
The PR adds:
Regarding the problem I was facing about refreshing the list, it has been worked out in a very hacky manner for the moment. I will explain the problem better in the meeting tomorrow. The gist is that it works when we open the app on some other menu tab and then navigate to the
Settings
tab. This allows me to call theviewWillAppear
method which takes care of the refresh. The refresh uses the Notification system. Publishes a notification whenever the value changes which is then handled by a function that is set to execute. This function calls the refresh on the child table views.I also tried to add a notification observer to only the dynamic data sections so that not all child tables are reloaded. This is done using an additional property
hasDynamicData
which accepts an enum value. Chose an enum since if we add more sections further that have dynamic data, handling it using a switch statement should be easier.I plan on working on this in the background. But this issue was holding back from moving on to the next issues so I decided to push this unoptimised yet best working state. Do let me know if it is okay for now.
Related issue