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

feat: render question tags (results, q/db-preview) #110

Merged
merged 6 commits into from
Feb 13, 2024
Merged

feat: render question tags (results, q/db-preview) #110

merged 6 commits into from
Feb 13, 2024

Conversation

lorenzocorallo
Copy link
Contributor

@lorenzocorallo lorenzocorallo commented Feb 5, 2024

Implement question tags rendering in results pdf, qpreview and dbpreview pages.
At the moment, the same tags are shown for every question in every section - database/data structure changes needed to conditionally show tags.
This PR is used to achieve the intended rendering style.

Closes #62

@github-actions github-actions bot temporarily deployed to preview February 5, 2024 17:04 Destroyed
Copy link

github-actions bot commented Feb 5, 2024

Pages Preview
Preview removed because the pull request was closed.
2024-02-13 00:18:44 +0000

Copy link
Member

@EndBug EndBug left a comment

Choose a reason for hiding this comment

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

Nice, I like the way it looks!
Just a couple of notes in the review comments ;)

src/components/Util/Question.tsx Outdated Show resolved Hide resolved
src/components/Util/Question.tsx Outdated Show resolved Hide resolved
@EndBug
Copy link
Member

EndBug commented Feb 5, 2024

Here is what I would do to add the tags to our database.
You need to edit the scripts/database.ts file, which is called when you run npm run build:db

You can add the tag column to the required headers:

const requiredHeaders = [
'ID',
'quesito',
'rispostaA',
'rispostaB',
'rispostaC',
'rispostaD',
'rispostaE',
'rispostaCorretta',
'immaginiQuesito'
]

Then you can updated the way the entries are created:

res[sheetDict[sheet.title]] = rows.map((r) => ({
id: r.ID,
sub: r.sub,
track: r.brano,
text: r.quesito,
answers: {
a: r.rispostaA,
b: r.rispostaB,
c: r.rispostaC,
d: r.rispostaD,
e: r.rispostaE
},
correct: r.rispostaCorretta?.toLowerCase(),
attachments:
DriveClient.matchFileIds(r.immaginiQuesito || '') || undefined,
validated: (r.validato as string | undefined)?.toLowerCase() == 'sì'
}))

Please keep in mind that the tag cells will contain a semicolon-separated list of tags: make sure to parse them correctly and store them as an array, so that we can do some grouping in the future ;)

@lorenzocorallo lorenzocorallo removed the request for review from zagbc February 5, 2024 21:19
@lorenzocorallo lorenzocorallo marked this pull request as ready for review February 5, 2024 21:25
@lorenzocorallo lorenzocorallo requested a review from EndBug February 5, 2024 21:25
@github-actions github-actions bot temporarily deployed to preview February 5, 2024 21:25 Destroyed
@lorenzocorallo
Copy link
Contributor Author

@EndBug I implemented the new column in our script.
I also removed the import and added the conditional rendering logic.
LGTY?

Copy link
Member

@EndBug EndBug left a comment

Choose a reason for hiding this comment

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

Hey, sorry for the delay

I'm getting an unexpected application error at runtime 👻
image

This happens in all the pages that show tags: it could be due to the fact that the current prod database doesn't contain the tags entry.
Addressing my other comment will probably result in fixing both issues ;)

scripts/database.ts Outdated Show resolved Hide resolved
@github-actions github-actions bot temporarily deployed to preview February 12, 2024 20:11 Destroyed
@github-actions github-actions bot temporarily deployed to preview February 12, 2024 20:14 Destroyed
@github-actions github-actions bot temporarily deployed to preview February 13, 2024 00:13 Destroyed
Copy link
Member

@EndBug EndBug left a comment

Choose a reason for hiding this comment

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

Alright, awesome!
I just pushed a quick fix because I noticed that the tags weren't trimmed when building the db ;)

tags: r.tag
? (r.tag as string).split(';').map((s) => s.trim())
: undefined

I think I'll just merge this to main, so that we can build the dev db from there
If there are some extra fixes needed, we can open a new PR

Thanks for the great work 🔥

@EndBug EndBug merged commit 100786a into main Feb 13, 2024
11 checks passed
@EndBug EndBug deleted the 62-tags branch February 13, 2024 00:18
@EndBug EndBug mentioned this pull request Feb 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add question tags for MAT and FIS
2 participants