-
Notifications
You must be signed in to change notification settings - Fork 144
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
fix: last acc hidden in switch acc list, closes #5975 #5987
base: dev
Are you sure you want to change the base?
Conversation
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.
Approved as these decisions were prior to your PR, but some of these hard coded values don't look like the way to do it
@@ -20,7 +20,7 @@ export function VirtuosoWrapper({ children, hasFooter, isPopup }: VirtuosoWrappe | |||
const [key, setKey] = useState(0); | |||
const isAtLeastMd = useViewportMinWidth('md'); | |||
const virtualHeight = isAtLeastMd ? '70vh' : '100vh'; | |||
const headerHeight = isPopup ? 230 : 60; | |||
const headerHeight = isPopup ? 300 : 80; |
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.
Seems like a bad choice to me that the core virtuoso component has these hard coded header/footer heights.
What if i use the component again with a different footer?
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 didn't want to start huge refactoring here
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 code isn't ideal but we have had this issue for a long time and this was the simplest fix. We only use this 3 times and twice with a footer.
We could refactor to pass the values in but I think it's OK
<HStack alignItems="center"> | ||
<PlusIcon /> | ||
Generate new account | ||
<styled.span textStyle="label.02">Generate new account</styled.span> | ||
</HStack> | ||
{component} |
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.
Shouldn't the container just be changed such that the height of the list stops when it gets to the button, and doesn't go under it?
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 for fixing this @alter-eggo 👍
I'm almost certain it was fixed before so sad to see it re-appear but glad you sorted it
This pr fixes last acc hidden in switch acc sheet and in choose acc flow