LF-5182: TAPE Plan Phase 1 - Frontend Changes#4068
Conversation
Generation plus one pass for type errors
There was a problem hiding this comment.
This looks pretty good to me.
TODOs:
getSurveyVersionneeds to be adjusted- eg.,
COUNTRY_VERSIONS.includes(countryCode) ? countryCode.toLowerCase() : 'default'
- eg.,
getTapeSurveyDataendpoint is missing- Replace
tapeQuestions.jsonwithuseQueryinTAPEResults - Delete
tapeQuestions.json Refetch is not happening when choosing a farm in a different country?- How do we want to handle errors?
- Test with the APIs
…ry in TapeResults
* add TapeRoutes and pass surveyVersion to each component * support only Australia version
4f80b54 to
de7b624
Compare
|
|
||
| const surveyVersion = getSurveyVersion(farm?.country_code); | ||
| const { data: tapeSurvey, isError: isTapeSurveyError } = useGetTapeSurveyQuery(); | ||
| const isTapeSurveyCompleted = !isTapeSurveyError && !!tapeSurvey?.id; |
There was a problem hiding this comment.
By following these steps:
Go to an Australian farm -> Complete a survey -> go to another Australian farm
You get tapeSurvey for the previous farm although the API is refetched and returns 404. isTapeSurveyError is to prevent the wrong "completed" status.
There was a problem hiding this comment.
Is there some blocking we could also do on isLoading in this view? When I switch farms, before the results data has come in (even without network throttling) I keep accidentally navigating to the survey (instead of the results view), as that is the link that is active initially until the query completes.
I think it would be better if there was no link until the query completes (or maybe no tile? I'm not really sure!) rather than give the opportunity to navigate to the wrong page. Being brought to the first page of the cleared survey made me think that the data was lost, and I think a user might think they have to take the whole survey again from the beginning, which would be pretty bad.
There was a problem hiding this comment.
When current data present for farm1 and error for farm2 it shows data on the wrong farm: This seems like another showcase for why it seems important to fixi the api state when switching between farms...
The root cause of the other farms data still being present can be fixed. @SayakaOno could you please check out : #3988 .
If an extension officer or anyone helps multiple farms fill out tape surveys this likelihood increases.
There was a problem hiding this comment.
@kathyavini Thank you so much for catching it! Tape's current data will be "Loading..." while fetching the results, and I disabled onClick using CSS. It would be appreciated if you could test the new UX 🙏
@Duncan-Brain I thought about that PR too. I've been looking for the right time for us to revisit it; let's do it tomorrow maybe after the release?
There was a problem hiding this comment.
@SayakaOno I love it! This is very clean and thank you so much for handling on those crazy scope changes in the last two days, that was a very fast adjust to all of that! 🙇🙇
The only blocking part is the inherited code for the results graph -- it shouldn't be (and in this particular case, doesn't work with) parsing the survey JSON. I am sorry; I threw that fact at you in a tech daily but I should have followed up with a link or written note!
packages/webapp/src/containers/Insights/TapeSurvey/TapeResults.tsx
Outdated
Show resolved
Hide resolved
packages/webapp/src/containers/Insights/TapeSurvey/TapeResults.tsx
Outdated
Show resolved
Hide resolved
|
|
||
| const surveyVersion = getSurveyVersion(farm?.country_code); | ||
| const { data: tapeSurvey, isError: isTapeSurveyError } = useGetTapeSurveyQuery(); | ||
| const isTapeSurveyCompleted = !isTapeSurveyError && !!tapeSurvey?.id; |
There was a problem hiding this comment.
Is there some blocking we could also do on isLoading in this view? When I switch farms, before the results data has come in (even without network throttling) I keep accidentally navigating to the survey (instead of the results view), as that is the link that is active initially until the query completes.
I think it would be better if there was no link until the query completes (or maybe no tile? I'm not really sure!) rather than give the opportunity to navigate to the wrong page. Being brought to the first page of the cleared survey made me think that the data was lost, and I think a user might think they have to take the whole survey again from the beginning, which would be pretty bad.
|
Seems to be working pretty well for me! I noticed a few things:
|
SayakaOno
left a comment
There was a problem hiding this comment.
Thank you so much for reviewing! 🙇 I think I've addressed everything, and the result chart should be responsive now.
Re-reviews would be appreciated 🙏
When I fill out the survey and accidentally press no to any terms and conditions. I cannot recover from that.
Do we need to handle this?
If so I am wondering if we should think about adding countries and the DIMENSION_MAPPING Joyce mentions in her comment above to the surveyJson metadata.
I agree, I'm expecting some changes in the future.
|
|
||
| const surveyVersion = getSurveyVersion(farm?.country_code); | ||
| const { data: tapeSurvey, isError: isTapeSurveyError } = useGetTapeSurveyQuery(); | ||
| const isTapeSurveyCompleted = !isTapeSurveyError && !!tapeSurvey?.id; |
There was a problem hiding this comment.
@kathyavini Thank you so much for catching it! Tape's current data will be "Loading..." while fetching the results, and I disabled onClick using CSS. It would be appreciated if you could test the new UX 🙏
@Duncan-Brain I thought about that PR too. I've been looking for the right time for us to revisit it; let's do it tomorrow maybe after the release?
|
@SayakaOno I approve this PR! Please merge whenever ready ❤️ You can also retag if you want to add any further responsivity adjusts first. I can't believe it -- I had just assumed the chart was non-responsive instead of trying throwing it in a responsive container 😭 The new UX of the loading state on the insights tile is perfect 👌 |
* Add insightContainer style to the wrappers * Update results chart labels style, add overflow scroll
|
Thank you @kathyavini for testing! I also found a bug that the survey is not available right after creating a farm in Australia. I'll open another PR for the fix. |
kathyavini
left a comment
There was a problem hiding this comment.
Oh that looks so much better!!! 🔥🔥🔥
I also found a bug that the survey is not available right after creating a farm in Australia. I'll open another PR for the fix.
Really?? I made a new farm when testing this yesterday. I'll check it out in the new PR 🙏
Please do approve and merge ☑️
| const words = label.split(' '); | ||
| const splitIndex = words.length === 1 ? 1 : Math.floor(words.length / 2); | ||
|
|
||
| return [words.slice(0, splitIndex).join(' '), words.slice(splitIndex).join(' ')]; |
There was a problem hiding this comment.
Oh it takes arrays for the labels! Thank you for figuring that out ❤️
Description
isCompletedstate and calculate it using saved survey (if exists, isCompleted is true)inProgressNote:
Jira link: https://lite-farm.atlassian.net/browse/LF-5182
Type of change
How Has This Been Tested?
Checklist:
pnpm i18nto help with this)