-
-
Notifications
You must be signed in to change notification settings - Fork 7.5k
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
settings_components: Split functions to improve type checking. #30090
Conversation
web/src/settings_org.js
Outdated
property_value = settings_components.get_stream_settings_property_value(property_name, sub); | ||
} else { | ||
property_value = settings_components.get_realm_settings_property_value(property_name); | ||
} |
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.
We should also split this function, like the others it's mainly a bunch of switch statements for individual settings and thus will do nicely being split up.
006a7d6
to
bb83d80
Compare
3a6790c
to
96b4987
Compare
96b4987
to
49cce92
Compare
Ready for review! Naming might be a concern because the names of the functions aren't very concise. |
web/src/settings_org.js
Outdated
update_dependent_subsettings(property_name); | ||
} | ||
|
||
export function discard_stream_proprty_element_changes(elem, sub) { |
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.
property
has a typo here.
I think this version does achieve our type-checking goals; @sahil839 should probably do a round of review and testing to make sure it doesn't regress anything. @afeefuddin how have you tested this? |
@afeefuddin can you also rebase this? Some reafactoring changes in the settings code have been merged recently. |
49cce92
to
2b19496
Compare
Done with rebasing @sahil839. One test is cancelled for some reason. I have tested a few functions manually, but I don't know how I should test this otherwise. |
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.
Posted a couple of comments. I did some testing and it works fine.
A good way of testing would to test all the sections where save-discard widget is used by verifying that the setting is changed, setting is discarded on clicking "Discard" and also by resetting the setting by selecting the same value after changing it once and check that the save-discard widget disappears as expected. |
Also, would it be worth moving functions for streams and groups to respective files like |
2b19496
to
179e035
Compare
I am not sure. I can do this in a follow-up commit if you suggest so. |
Merged, thanks @afeefuddin and @sahil839! @afeefuddin can you be sure to do the testing suggested in the next few days and post the results in your checkins? I wasn't sure if you'd done that yet. Regarding moving the code, I think if we can move them without creating messy input cycles, yes, having a |
Refactor get_property_value and check_property_changed to improve type checking and code readability.
Self-review checklist
(variable names, code reuse, readability, etc.).
Communicate decisions, questions, and potential concerns.
Individual commits are ready for review (see commit discipline).
Completed manual review and testing of the following: