Refactor of WorkspaceReimburseView to use new API #10692
Conversation
melvin-bot
bot
requested review from
marcochavezf
and removed request for
a team
|
@arosiclair Have we considered the offline behavior for this? Currently as set up if we're offline we just show a 0.000 rate and
|
yuwenmemon
force-pushed
the
yuwen-workspaceReimbursePage
branch
from
83f626b to
a90120b
Compare
This is for when you have not previously viewed the custom unit while online correct? If so, that's expected since we don't have any rate saved in Onyx to display. If you did previously view the custom unit while online, then the same value should display after going offline. |
Yep, that's correct - and sounds good! This is ready to be reviewed |
| if (prevProps.policy.customUnits !== this.props.policy.customUnits) { | ||
| const distanceCustomUnit = _.chain(lodashGet(this.props, 'policy.customUnits', [])) | ||
| .values() | ||
| .findWhere({name: CONST.CUSTOM_UNITS.NAME_DISTANCE}) | ||
| .value(); | ||
|
|
||
| this.setState({ | ||
| unitID: lodashGet(distanceCustomUnit, 'customUnitID', ''), | ||
| unitName: lodashGet(distanceCustomUnit, 'name', ''), | ||
| unitValue: lodashGet(distanceCustomUnit, 'attributes.unit', 'mi'), | ||
| rateID: lodashGet(distanceCustomUnit, 'rates[0].customUnitRateID', ''), | ||
| rateName: lodashGet(distanceCustomUnit, 'rates[0].name', ''), | ||
| rateValue: this.getRateDisplayValue(lodashGet(distanceCustomUnit, 'rates[0].rate', 0) / 100), | ||
| }); | ||
| } |
There was a problem hiding this comment.
NAB: Why don't we use the props directly? I think it will be a bit hard to keep props and state in sync if we add more props in the future, no?
There was a problem hiding this comment.
Yeah, I'm trying to have as little impact on this aspect of the page as possible and simply support the new API command. It's been worked on by a number of contributors based on what I've seen. I agree it could use some improvement in how it's written but I feel like that would be the domain of a follow-up issue.
|
I noticed that after refreshing the browser, the unit value changes quickly to the previous value (Kilometers -> Miles -> Kilometers), I wonder if it would be related to the props - state sync Screen.Recording.2022-08-31.at.12.47.39.mov |
|
Yeah, cc @arosiclair but I would guess that's by design since the doc states we use |
There was a problem hiding this comment.
It looks like the componentDidUpdate logic is conflicting with getDerivedStateFromProps since they're both trying to set the same state. I tested removing getDerivedStateFromProps and it fixed the kilometers-miles flickering on refresh so I think we should just remove it
|
Sounds good, and added! |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by @arosiclair in version: 1.1.97-0 🚀
|
|
Tester is facing a different issue now when re-testing. the error doesn't disappear after appearing. We are collecting more info to see if it needs to be logged Recording.1797.mp4 |
@arosiclair please review
cc @ctkochan22
Details
Refactor WorkspaceReimburseView to use the new API
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/217856
Tests/QA