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

No questions remaining UI #56

Closed
wants to merge 4 commits into from
Closed

No questions remaining UI #56

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cef4de6
chore: hunger game changed to hunger games
sumana2001 Jun 22, 2022
357b084
Merge branch 'openfoodfacts:master' into master
sumana2001 Jul 7, 2022
63fed89
enhancement: no questions remaining
sumana2001 Jul 17, 2022
3dafa43
Update src/pages/questions/QuestionDisplay.jsx
sumana2001 Jul 21, 2022
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
58 changes: 56 additions & 2 deletions src/pages/questions/QuestionDisplay.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,13 +57,67 @@ const getValueTagExamplesURL = (question) => {
return "";
};

const QuestionDisplay = ({ question, answerQuestion }) => {
const QuestionDisplay = ({ question, answerQuestion, resetFilters }) => {
const { t } = useTranslation();
const valueTagQuestionsURL = getValueTagQuestionsURL(question);
const valueTagExamplesURL = getValueTagExamplesURL(question);

if (question === NO_QUESTION_LEFT) {
return <p>{t("questions.no_questions_remaining")}</p>;
return (
<Stack alignItems="center" spacing={2}>
<Stack
direction={{ xs: "column", sm: "row" }}
alignItems="center"
spacing={{ xs: 0, sm: 2 }}
>
<h2>{t("questions.no_questions_remaining")}</h2>
<Button size="small" variant="contained" onClick={resetFilters}>
Reset filters
</Button>
</Stack>
<h1>OR</h1>
Copy link
Member

Choose a reason for hiding this comment

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

<h1> has a meaning: this is the top title of the page. I assume the top title of this page is not "OR" 😉

Same for h2, h3, ... If you want a big text use CSS, not HTML

On this topic, you can read <Typography/> doc

<Typography variant="h1" component="p">A big text</Typography>

This components hase two important props:

  • variant: defines how the text will look like (here it will look like a top title)
  • componentthe HML compont which will be used (the sementic meaning (here it's a basic text)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh okay, got it. Thank you. I'll make the changes

<br />
<h2 style={{ textAlign: "center" }}>
Install the app and contribute even more
</h2>
<Stack direction="row" spacing={1}>
<a href="https://apps.apple.com/app/open-food-facts/id588797948">
<img
src="https://world.openfoodfacts.org/images/misc/appstore/black/appstore_US.svg"
alt="Appstore"
width="140px"
height="60px"
/>
</a>
<a href="https://play.google.com/store/apps/details?id=org.openfoodfacts.scanner&hl=en">
<img
src="https://static.openfoodfacts.org/images/misc/google-play-badge-svg-master/img/en_get.svg"
alt="Appstore"
width="140px"
height="60px"
/>
</a>
</Stack>
<Stack direction="row" spacing={1}>
<a href="https://www.microsoft.com/en-us/p/openfoodfacts/9nblggh0dkqr">
<img
src="https://world.openfoodfacts.org/images/misc/microsoft/English.svg"
alt="Appstore"
width="140px"
height="60px"
/>
</a>
<a href="https://world.openfoodfacts.org/files/off.apk">
<img
src="https://static.openfoodfacts.org/images/misc/android-apk.svg"
alt="Android"
width="140px"
height="60px"
/>
</a>
</Stack>
</Stack>
);
}
if (question === null) {
return <p>loading</p>;
Expand Down
10 changes: 10 additions & 0 deletions src/pages/questions/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,15 @@ export default function Questions() {
useQuestionBuffer(filterState);
const question = buffer[0] ?? null;

const resetFilters = React.useCallback(
Copy link
Collaborator

Choose a reason for hiding this comment

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

useCallback (memoization) may not be needed in this case, maybe go with a normal arrow function?

Copy link
Member

Choose a reason for hiding this comment

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

The setFilterState is not usefull in the array of dependencies, because state setter are guaranteed to stay constant between render

It's not entirely useless, because resetFilters is passed to QuestionDisplay. WIthout that useCallback each render will create a new function, and so <QuestionDisplay/> will rerender too

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I guess that the baseline is if the useCallback memoization improves performance. I think that in this case as the QuestionDisplay is a presentational component and doesn't have heavy computations or something really slow, there is not a lot of benefit in memoizing the reset function and makes the code a bit more complex. Also, the memoization is not cost free, it will store in memory and every render needs to compare with dependency array. Besides that, the QuestionDisplay will rerender every time the Questions (index) rerenders, unless QuestionDisplay itself is memoized.

() =>
setFilterState((prevState) => ({
insightType: prevState.insightType,
sortByPopularity: prevState.sortByPopularity,
})),
[setFilterState]
);

return (
<Grid container spacing={2} p={2}>
<Grid item sm={12} md={5}>
Expand All @@ -30,6 +39,7 @@ export default function Questions() {
<QuestionDisplay
question={question}
answerQuestion={answerQuestion}
resetFilters={resetFilters}
/>
</Stack>
</Grid>
Expand Down