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.

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

Disables timezone options if it is set to automatic #15176

Merged
56 changes: 50 additions & 6 deletions src/pages/settings/Profile/TimezoneSelectPage.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,27 +34,71 @@ 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 = this.getUserTimezone(props.currentUserPersonalDetails);
this.allTimezones = _.chain(moment.tz.names())
.filter(timezone => !timezone.startsWith('Etc/GMT'))
.map(timezone => ({
text: timezone,
keyForList: timezone,
keyForList: this.getKey(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() {
// componentDidUpdate is added 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.
const newTimezone = this.getUserTimezone(this.props.currentUserPersonalDetails);
if (_.isEqual(this.timezone, newTimezone)) {
return;
}
this.timezone = newTimezone;
this.allTimezones = _.map(this.allTimezones, (timezone) => {
const text = timezone.text.split('-')[0];
return {
text,
keyForList: this.getKey(text),

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

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

Choose a reason for hiding this comment

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

We have a similar object structured within the constructor. We could probably create a function to return the object with a single argument.

});

this.setState({
timezoneInputText: this.timezone.selected,
timezoneOptions: this.allTimezones,
});
}
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 {String} text
* @return {string} key for list item
*/
getKey(text) {
return `${text}-${(new Date()).getTime()}`;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add a description of why we are doing this?


/**
* @param {Object} currentUserPersonalDetails
* @return {Object} user's timezone data
*/
getUserTimezone(currentUserPersonalDetails) {
return lodashGet(currentUserPersonalDetails, 'timezone', CONST.DEFAULT_TIME_ZONE);
}

/**
* @param {Object} timezone
* @param {String} timezone.text
Expand Down Expand Up @@ -90,7 +134,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