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

Updates based on the UX feedback #1187

Merged
merged 4 commits into from
Nov 8, 2024
Merged

Conversation

zhx828
Copy link
Member

@zhx828 zhx828 commented Nov 4, 2024

@zhx828 zhx828 added the fix Fix tag for release label Nov 4, 2024
@zhx828 zhx828 requested a review from calvinlu3 November 4, 2024 21:30
Comment on lines 164 to 168
<div
className={classNames(
'container',
styles.container,
isSticky ? styles.containerSticky : ''
isSticky ? styles.containerSticky : '',
'container'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Whats the difference between the class and the component?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question. Initially I removed the container, then added back the class. Let me try to revert to the component.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The icon inside the circle doesn't seem to be centered. Seems to be slightly leaning left. Do you see that too?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh yeah, it might relate to some changes after introducing the icon. I will look inot.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure why I can't reproduce locally.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was looking at the screenshots, but locally it is fine for me too.

Copy link
Collaborator

@calvinlu3 calvinlu3 left a comment

Choose a reason for hiding this comment

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

LGTM. Screenshots still failing.

className={classNames(
'container',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can remove class now

@zhx828 zhx828 changed the title Update based on the UX feedback Updates based on the UX feedback Nov 8, 2024
@zhx828 zhx828 merged commit a9db5f2 into oncokb:master Nov 8, 2024
6 checks passed
@zhx828 zhx828 deleted the minor-ux-updates branch November 8, 2024 13:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix Fix tag for release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants