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): added shadows to popup buttons and course blocks #378

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

Conversation

aaronthechen
Copy link
Contributor

@aaronthechen aaronthechen commented Oct 22, 2024

Resolves #351.

Before:

Screen.Recording.2024-10-21.at.10.25.47.PM.mov

After:

Screen.Recording.2024-10-21.at.10.22.45.PM.mov
  • Shadows are a bit faint for the course block, but they are the exact same as those for course hover in the calendar
  • There was an issue with the last course block's shadow, so I modified the overflow of List from hidden to clip, not sure if that is the best practice but I'm not sure if there are alternative methods

Huly®: UTRP-362


This change is Reviewable

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.

LGTM

@IsaDavRod IsaDavRod requested a review from Razboy20 October 22, 2024 04:22
@IsaDavRod
Copy link
Member

Requested Elie's review. It would be nice if all buttons on hover had a better transition tho (bc the shadow is so abrupt) (but that is a general comment not a comment for this PR)

@DereC4 DereC4 added the UI/UX-figma PRs that fulfill a task on the UI/UX & Feature Roadmap label Oct 22, 2024
@DereC4 DereC4 added this to the v2.1.0 milestone Oct 22, 2024
@@ -102,7 +102,7 @@ function List<T>({ draggables, itemKey, children, onReordered, gap }: ListProps<
);

return (
<div style={{ overflow: 'hidden' }}>
<div style={{ overflow: 'clip', overflowClipMargin: `${gap}px` }}>
Copy link
Member

Choose a reason for hiding this comment

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

What's the reasoning behind this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mentioned at the end of my PR but the last shadow in the course list was not shown due to the negative bottom margin in the List component and the hiding of the overflow, those two parts essentially cut the shadow from being visible

Copy link
Contributor Author

@aaronthechen aaronthechen Oct 23, 2024

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 method is likely the best method but I'm not sure of another way to resolve it, it essentially limits the overflow rather than completely hiding it, such that the shadow can be shown in the gap's clip

@@ -63,7 +63,7 @@ export default function PopupCourseBlock({
backgroundColor: colors.primaryColor,
}}
className={clsx(
'h-full w-full inline-flex items-center justify-center gap-1 rounded pr-3 focusable cursor-pointer text-left',
'h-full w-full inline-flex items-center justify-center gap-1 rounded pr-3 focusable cursor-pointer text-left hover:shadow-md transition-shadow-100 ease-out',
Copy link
Member

Choose a reason for hiding this comment

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

When dragging a course block, pointer events are not propagated, thus the hover shadow goes away.

Don't remember off the top of my head what the right way is; ping me if you can't figure it out quickly so I can give better advice.

@aaronthechen aaronthechen requested a review from Razboy20 November 7, 2024 01:10
@DereC4
Copy link
Member

DereC4 commented Nov 17, 2024

help is on the way

@IsaDavRod IsaDavRod linked an issue Nov 26, 2024 that may be closed by this pull request
icon={CalendarIcon}
/>
<Button variant='single' color='ut-black' onClick={handleOpenOptions} icon={SettingsIcon} />
<Button variant='single' color='ut-black' onClick={handleCalendarOpenOnClick} icon={Feedback} />
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is the feedback button, but the onClick was for the calendar page

Suggested change
<Button variant='single' color='ut-black' onClick={handleCalendarOpenOnClick} icon={Feedback} />
<Button variant='single' color='ut-black' onClick={openReportWindow} icon={Feedback} />

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