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

feat(ui): calendar header redesign #479

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

Preston-Cook
Copy link
Contributor

@Preston-Cook Preston-Cook commented Jan 1, 2025

This is a WIP for the modified toolbar on the calendar. I want to get some feedback on the code to see if there's a more efficient way to achieve this. Thanks!


This change is Reviewable

@doprz doprz self-requested a review January 2, 2025 04:03
@doprz doprz added wip feature UI/UX-figma PRs that fulfill a task on the UI/UX & Feature Roadmap labels Jan 2, 2025
Copy link
Member

@IsaDavRod IsaDavRod left a comment

Choose a reason for hiding this comment

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

Ensure all gaps/padding is using the new spacing system values from the Figma. See PR #474.

Another note is that the buttons are supposed to be of the minimal variant but that hasnt been implemented yet in a different Issue #470.

Also, does the responsiveness break if we were to add or remove a feature button from the toolbar?


return (
<div className='flex items-center gap-5 overflow-x-auto overflow-y-hidden border-b border-ut-offwhite px-7 py-4 md:overflow-x-hidden'>
<div className='flex items-center gap-5 overflow-x-auto overflow-y-hidden border-b border-ut-offwhite py-5 pl-6 md:overflow-x-hidden'>
<Button
variant='single'
icon={Sidebar}
color='ut-gray'
Copy link
Member

@IsaDavRod IsaDavRod Jan 3, 2025

Choose a reason for hiding this comment

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

  • Change the button color to theme-black for now (Ethan's Calendar Sidebar PR already has it black too)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right now, yes, adding more buttons would break it. I can try to find workarounds, though. I have a few ideas.

Comment on lines 102 to 105
<>
<LargeLogo className='flex-shrink-0' />
<Divider className='mx-2 flex-shrink-0 self-center md:mx-4' size='2.5rem' orientation='vertical' />
</>
Copy link
Member

@IsaDavRod IsaDavRod Jan 3, 2025

Choose a reason for hiding this comment

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

  • Since the logo is moved to the calendar sidebar, we can remove it from the header. This divider would also be removed.

<Button variant='single' icon={RedoIcon} color='ut-black' /> */}
<Button variant='single' icon={GearSix} color='theme-black' onClick={handleOpenOptions} />

<Divider size='2.5rem' orientation='vertical' />
Copy link
Member

@IsaDavRod IsaDavRod Jan 3, 2025

Choose a reason for hiding this comment

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

  • Change divider height/size to 1.75rem and use the theme-offwhite1 color (assuming your color PR is merged in main)

</Menu>
</DialogProvider>
</div>
<Divider size='2.5rem' orientation='vertical' />
Copy link
Member

@IsaDavRod IsaDavRod Jan 3, 2025

Choose a reason for hiding this comment

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

  • Change divider height/size to 1.75rem and use the theme-offwhite1 color (assuming your color PR is merged in main)

</>
)}

<div className='min-w-0 screenshot:transform-origin-left screenshot:scale-120'>
Copy link
Member

@IsaDavRod IsaDavRod Jan 3, 2025

Choose a reason for hiding this comment

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

  • This container should have a minimm width of 10.9375rem to prevent the following overlap:
    screenshot 2025-01-02 at 18 32 04@2x


return (
<div className='flex items-center gap-5 overflow-x-auto overflow-y-hidden border-b border-ut-offwhite px-7 py-4 md:overflow-x-hidden'>
<div className='flex items-center gap-5 overflow-x-auto overflow-y-hidden border-b border-ut-offwhite py-5 pl-6 md:overflow-x-hidden'>
Copy link
Member

Choose a reason for hiding this comment

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

  • Remove the offwhite border since the toolbar/header will no longer have a bottom border.

@IsaDavRod IsaDavRod linked an issue Jan 14, 2025 that may be closed by this pull request
5 tasks
@Preston-Cook Preston-Cook changed the title feat(ui): calendar header redesign (WIP) feat(ui): calendar header redesign Jan 14, 2025
Copy link
Member

@Razboy20 Razboy20 left a comment

Choose a reason for hiding this comment

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

There is a lot of added dynamic Javascript code to deal with screen sizes, and hiding text. This should be done entirely in CSS by either using media queries (in tailwind) or container queries. Keep in mind that accessibility is also important, so this text should be hidden on displays but still read on screen-readers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature UI/UX-figma PRs that fulfill a task on the UI/UX & Feature Roadmap wip
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Modify the Calendar Toolbar with the new Figma design
4 participants