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.

Sign up for GitHub

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

Jump to bottom

Disables timezone options if it is set to automatic #15176

Merged
merged 7 commits into from
Feb 28, 2023
Merged

Disables timezone options if it is set to automatic #15176

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0d34efd
disabled timezone options if it is set to automatic
priyeshshah11 Feb 15, 2023
073f8f7
updated the comment in componentDidUpdate
priyeshshah11 Feb 16, 2023
9e22ee5
refactored componentDidUpdate to fix options not udating issue
priyeshshah11 Feb 16, 2023
3f587b6
refactored get timezone code to follow DRY
priyeshshah11 Feb 16, 2023
953ca2e
Code refactoring to follow DRY & add comments
priyeshshah11 Feb 19, 2023
97c20b3
minor code cleanup
priyeshshah11 Feb 19, 2023
e27a3ae
merged master & resolved conflicts
priyeshshah11 Feb 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 26 additions & 5 deletions src/pages/settings/Profile/TimezoneSelectPage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,27 +34,48 @@ class TimezoneSelectPage extends Component {
this.saveSelectedTimezone = this.saveSelectedTimezone.bind(this);
this.filterShownTimezones = this.filterShownTimezones.bind(this);

this.currentSelectedTimezone = lodashGet(props.currentUserPersonalDetails, 'timezone.selected', CONST.DEFAULT_TIME_ZONE.selected);
this.timezone = lodashGet(props.currentUserPersonalDetails, 'timezone', CONST.DEFAULT_TIME_ZONE);
this.allTimezones = _.chain(moment.tz.names())
.filter(timezone => !timezone.startsWith('Etc/GMT'))
.map(timezone => ({
text: timezone,
keyForList: timezone,

// Include the green checkmark icon to indicate the currently selected value
customIcon: timezone === this.currentSelectedTimezone ? greenCheckmark : undefined,
customIcon: timezone === this.timezone.selected ? greenCheckmark : undefined,

// This property will make the currently selected value have bold text
boldStyle: timezone === this.currentSelectedTimezone,
boldStyle: timezone === this.timezone.selected,
}))
.value();

this.state = {
timezoneInputText: this.currentSelectedTimezone,
timezoneInputText: this.timezone.selected,
timezoneOptions: this.allTimezones,
};
}

componentDidUpdate() {
// Update timezoneInputText & all timezoneOptions when the timezone object changes
const newTimezone = lodashGet(this.props.currentUserPersonalDetails, 'timezone', CONST.DEFAULT_TIME_ZONE);
if (_.isEqual(this.timezone, newTimezone)) { return; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (_.isEqual(this.timezone, newTimezone)) { return; }
if (_.isEqual(this.timezone, newTimezone)) {
return;
}

this.timezone = newTimezone;
const updatedAllTimezones = _.map(this.allTimezones, timezone => ({
...timezone,

// Include the green checkmark icon to indicate the currently selected value
customIcon: timezone === this.timezone.selected ? greenCheckmark : undefined,

// This property will make the currently selected value have bold text
boldStyle: timezone === this.timezone.selected,
}));

this.setState({
timezoneInputText: this.timezone.selected,
timezoneOptions: updatedAllTimezones,
});
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@priyeshshah11 Why do we need this componentDidUpdate?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It also adds componentDidUpdate in order to update the timezone options when automatic is toggled on/off as navigating back doesn't unmount the page, thus it won't update the timezone options & stay disabled without this.

We should put that instead of // Update timezoneInputText & all timezoneOptions when the timezone object changes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mollfpr

It also adds componentDidUpdate in order to update the timezone options when automatic is toggled on/off as navigating back doesn't unmount the page, thus it won't update the timezone options & stay disabled without this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @priyeshshah11 that will help to explain what's going on 👍


/**
* @param {Object} timezone
* @param {String} timezone.text
Expand Down Expand Up @@ -90,7 +111,7 @@ class TimezoneSelectPage extends Component {
onChangeText={this.filterShownTimezones}
onSelectRow={this.saveSelectedTimezone}
optionHoveredStyle={styles.hoveredComponentBG}
sections={[{data: this.state.timezoneOptions}]}
sections={[{data: this.state.timezoneOptions, isDisabled: this.timezone.automatic}]}
shouldHaveOptionSeparator
safeAreaPaddingBottomStyle={safeAreaPaddingBottomStyle}
/>
Expand Down