-
Notifications
You must be signed in to change notification settings - Fork 3
adds partner organizations to map and list #99
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
Conversation
for more information, see https://pre-commit.ci
✅ Deploy Preview for sbs-austria ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
src/routes/+layout.ts
Outdated
@@ -7,13 +7,16 @@ export const load: LayoutLoad = async () => { | |||
const nav = await fetch_yaml(`Nav`) | |||
const footer = await fetch_yaml(`Footer`) | |||
const social = await fetch_yaml(`Social`) | |||
const chapters = await fetch_chapters() | |||
var chapters = await fetch_chapters() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't use var
. Prefer let
or const
.
src/lib/ChapterMap.svelte
Outdated
`chapter`, | ||
chap.acceptsSignups ? `active` : chap.partnerAssociation ? 'partner' : `starting`, | ||
], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar suggestion to #98, how about instead of defining a bunch of booleans, we add an attribute status: 'active' | 'starting' | 'partner'
or similar to the Chapter
type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But we keep the bunch of booleans in Contentful right? There is no nice way to implement that you can select from a set of possible states.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
acceptSignups is the boolean variable that is currently directly loaded from Contentful. Maybe we should add an additional state for the signupsState?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I second this, this would be more elegant and easier to modify in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but I think the points @janosh mentioned (esp. with the contentful-interaction) would be much more elegant
58d92b6
to
29af134
Compare
Ok, I changed everything which was in question. So now we can merge it. |
Can be merged, if you think it's fine.
We could discuss about the color for partner organisations on the map.