-
Notifications
You must be signed in to change notification settings - Fork 0
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
encapsulate hub head block #43
Conversation
- addded font-size variables; - added new typeface variables; - move the "CSS Reset" outside of root, which was breaking elements such as the header navbar;
- created general fetcher utility; - added graphQL query to fetch all content;
…rs/setup-hub-page
…rs/setup-hub-page
- minor changes to css globals;
…rs/add-contentgrid-navbar
- rename variables for consistency, - improved typing,
- repurpose Settings collection to define highlights; - update graphQL queries to reflect changes; - style ContentPilland AuthorPill components; - fix icons svg fields to be compliant with React;
…rs/encapsulate-hub-head-block
- moved content nav bar together with content grid; - css styling fixes and changes;
<h6> {title} </h6> | ||
<div className={styles.metadataContainer}> | ||
{/* TODO: is this a good approach for multi category items? */} | ||
{categories.length > 1 ? ( |
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.
Does this make sense? Why not just always do the map?
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.
Yeah, either reduce the gap, or limit to n categories, or split into 2 rows on smaller screens, it depends 😂
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.
ok, sounds like a plan! i'll add it to the issue list
<ArchiveButton collection={archiveMap[contentType]} /> | ||
<h6>{title}</h6> | ||
<p>{summary}</p> | ||
|
||
{Array.isArray(categories) && categories.length > 0 |
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.
yep, it's giving me some headaches as TS is going crazy about that. could we enforce just (for example) Category[] and remove the string part?
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 the ideal. the problem is that this is generated by payload from the collection configs, and they look fine to me. I might look into this later
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.
ok, will leave it as is for now
<div | ||
className={styles.contentGridContainer} | ||
style={{ | ||
borderColor: windowWidth >= 1024 ? colorMap[activeButton] : 'var(--dark-rock-800)', |
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.
This would be better done in pure CSS, with media queries
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.
would this be a case for clsx?
change class based on active button just for desktop?
@@ -25,6 +26,12 @@ export default function HubContentGrid({ articles }: HubContentGridProps) { | |||
|
|||
const [activeButton, setActiveButton] = useState<keyof ContentTypeArrays | 'All'>('All') | |||
|
|||
let windowWidth = window.innerWidth | |||
|
|||
window.addEventListener('resize', () => { |
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 don't think this is necessary (because of the other comment about media queries), but anyway, event listeners in React need some extra care because they need to be cleaned up when the component unmounts. The best practice looks like this: https://www.pluralsight.com/resources/blog/guides/event-listeners-in-react-components
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.
thanks. i'll update it and shift to using pure css
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.
fixed in: f4f6c9e
…rs/encapsulate-hub-head-block
Why:
How: