-
Notifications
You must be signed in to change notification settings - Fork 1
Adding tab component #340
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
base: develop
Are you sure you want to change the base?
Adding tab component #340
Conversation
✅ Deploy Preview for gbof-c19nyc-staging ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for padp-staging ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for pss-scavenger-hunt ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for juel-staging ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
| } | ||
|
|
||
| <script is:inline> | ||
| window.document.addEventListener('DOMContentLoaded', () => { |
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.
What are you thoughts on using a React component with client:load vs. inline Javascript? Is the performance savings worth the readability/maintainability penalty?
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 did it this way because of the annoying restrictions on including Astro components inside React components; I want to be able to use the other page builder components as tab content and those are (mostly) Astro components. If we're happy restricting the tab content to, say, a couple of columns of rich text, or even to the specific name/title format of the team member tab designs, then it would make sense to do it as a React component, but my initial thought was to make the component a more flexible container for other 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.
That's a fair point. After doing some brain storming about it, I don't have a great solution. But I do wonder if we'll run into any side effects of spreading the interactivity logic between React and vanilla JS.
Do we want to opt out of script processing by using the is:inline directive? What happens if multiple Tabs sections are inserted into a page?
If we're allowing other page builder components to be added as tab content, will this require us to duplicate the page builder config options in Tina multiple times?
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.
Yeah good questions. It's definitely not ideal. I was envisioning perhaps eventually separating out some of the pages Tina schema into like, layout templates and content templates, and then the content templates list can more easily be included in multiple places. (This is how e.g. Storyblok, which we used for NBU, organizes its content blocks; when you create a block you can specify it as like page, layout, content, or some combination, and that affects where it can be inserted.) Definitely something to think about once we have the full complement of components.
Hmm, I think the script could be modified to handle the possibility of multiple tab components on one page; that's a good catch.
In this PR
Adds the tabbed content component as described in Issue #311 .
Notes and questions
Container.container), but I held off on that to avoid unnecessarily complication; certainly for the use case in the JUEL designs having themulti_columntemplate is sufficient. But I'm open to other suggestions.