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

Add basic banner functionality #300

Merged
merged 1 commit into from
Aug 8, 2023
Merged

Add basic banner functionality #300

merged 1 commit into from
Aug 8, 2023

Conversation

mruwnik
Copy link
Collaborator

@mruwnik mruwnik commented Aug 6, 2023

#292

image

This is just adds the basic functionality, without any decent colours or images (both of which can be set in Coda)

app/routes/questions/$question.tsx Show resolved Hide resolved
@@ -263,6 +288,10 @@ export const loadQuestionDetail = withCache('questionDetail', async (question: s
const {data} = await loadTags('NEVER_RELOAD')
allTags = Object.fromEntries(data.map((r) => [r.name, r])) as Record<string, Tag>
}
if (Object.keys(allBanners).length === 0) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

isn't this fun...

Copy link
Collaborator

Choose a reason for hiding this comment

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

feel free to create an issue where you collect all notes about what needs to be double checked and loaded in correct order currently and deserves simplification, I might get to it during aisafety.com hackathon 🤞

tags,
} = questionProps
const title =
codaStatus && codaStatus !== QuestionStatus.LIVE_ON_SITE ? `WIP - ${codaTitle}` : codaTitle
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

a banner can be displayed for non published questions, rather than the WIP -

Copy link
Collaborator

Choose a reason for hiding this comment

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

"can be" or "is" actually displayed for all current WIP questions?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

they all have the appropriate banner set, so it's mainly a matter of deciding whether that's the way to go. The parser is already updated to do so

Copy link
Collaborator

Choose a reason for hiding this comment

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

color scheme in the current screenshot would not be my first choice, but if it gives more power to the editors to customize the banner instead of hardcoded WIP -, it sounds like a step in good direction 👍

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, this most certainly is a bad choice - I pretty much just plopped random colours there to "encourage" someone else to choose decent ones :D

tags,
} = questionProps
const title =
codaStatus && codaStatus !== QuestionStatus.LIVE_ON_SITE ? `WIP - ${codaTitle}` : codaTitle
Copy link
Collaborator

Choose a reason for hiding this comment

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

"can be" or "is" actually displayed for all current WIP questions?

app/routes/questions/$question.tsx Show resolved Hide resolved
@@ -263,6 +288,10 @@ export const loadQuestionDetail = withCache('questionDetail', async (question: s
const {data} = await loadTags('NEVER_RELOAD')
allTags = Object.fromEntries(data.map((r) => [r.name, r])) as Record<string, Tag>
}
if (Object.keys(allBanners).length === 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

feel free to create an issue where you collect all notes about what needs to be double checked and loaded in correct order currently and deserves simplification, I might get to it during aisafety.com hackathon 🤞

@@ -263,6 +288,10 @@ export const loadQuestionDetail = withCache('questionDetail', async (question: s
const {data} = await loadTags('NEVER_RELOAD')
allTags = Object.fromEntries(data.map((r) => [r.name, r])) as Record<string, Tag>
}
if (Object.keys(allBanners).length === 0) {
const {data} = await loadBanners('NEVER_RELOAD')
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is the only place where it's called? NEVER_RELOAD doesn't sound right, normal cache should be used with reloading in background after 10 minutes IMHO 🤔

@mruwnik mruwnik merged commit 2f9cd81 into master Aug 8, 2023
1 check passed
@mruwnik mruwnik deleted the banners branch August 8, 2023 09:49
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.

2 participants