-
Notifications
You must be signed in to change notification settings - Fork 65
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 session types content #989
Conversation
for more information, see https://pre-commit.ci
Preview available
|
Hi Raquel, thanks for the PR! I think it looks great. Little extra note: It might be nice if we extract the code into components in the future so it's less work for the team. Let's actually open a meta issue for the components needed, so we have a list to work on. In the meantime, I think using this style/code is perfectly good. |
@egeakman thanks for the feedback! <3
100%! That’s largely why I held back from typing away. Repeating all the styling is a pain to type and for content reviewers! How about I open a separate PR for the accordion component? (I’m hoping to first get some accessibility team's feedback). That way, we can tick off one component from the future list. Once the component is ready, I can merge it into this content PR. Since this content is an improvement/good-to-have, we can afford to take the time to do it well. WDYT? |
While working on the accordion styling for session types in [PR #989](#989), it became clear that extracting it into a reusable component would make things easier for the team. --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Ege Akman <[email protected]>
@hypha how do you think we should proceed on this? cc @ADubhlaoich for accessibility |
Thanks for the ping @egeakman ! Yes I think so. I just haven't had the time to finish this. I am hoping next week. Sorry for the delay! |
No worries! Thanks for working on it ❤️ |
Implemented in PR #1073 ✅ |
Hi @clytaemnestra, thanks for implementing the accordion style with Marcin in #1073. The main purpose of this PR was to add the content of the session types. The accordion style was to make it look a bit better. Now the accordion style is no longer needed. But I wonder if you meant to add the session type content on #1073 as well. Because otherwise, I was planning to finish the content here after merging the styling of 1073 if it goes ahead. |
…nd Events; flatten style
for more information, see https://pre-commit.ci
As agreed with @clytaemnestra, I have removed all the styling (as FAQ has changed to accordion style). Instead, I just prepared the content for the session types details. I separated each type with "---" purely to be more readable right now. Mia, if you have specific instructions for where to put the content, or how to style it, happy to follow. Or you can also take it with @nikoshell to be implemented elsewhere. For now, it would be good to get reviews over the content. |
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.
All looks good to me, thank you Raquel! ❤️
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, thanks a lot, @hypha!
Thanks for the reviews, Ege and Mia! |
I'd merge the PR and add styling on the program page, if we move it there. I don't have a clear vision; in general I'd like to have these kind of info on the program page and only links on the FAQ, but I'd have to see it content-wise. So, let's maybe leave it as is and refactor later, if needed 🙂 |
Fixes #985