-
Notifications
You must be signed in to change notification settings - Fork 275
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 for aragonUI and other clean up #939
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.
💥
key={index} | ||
href={`#${getAppPath({ | ||
dao, | ||
instanceId: 'permissions', | ||
params: `app.${value.proxyAddress}`, | ||
})}`} | ||
target="_blank" | ||
focusRingSpacing={[2, 2]} |
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.
Is this needed? I was more seeing it as an internal prop that Link
uses, but if we need to use it, maybe we should make it part of Link
prop types and document 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.
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.
mmm yes maybe we should fix the default then. I agree the first one could be improved, but the second one is too close to the characters IMO.
It might be a good case for em
, so that it would depend on the text size… but the problem is that I don’t think there is a way to stop depending on the text size after a certain point (we don’t want huge spacing on large titles). But we could do some tests at different text sizes, maybe using a combination of px
and em
using calc()
, and see if it works.
horizontalPadding="none" | ||
css="padding: 0" | ||
> | ||
<Link href={url} focusRingSpacing={[2, 2]}> |
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.
Same here for focusRingSpacing
.
…tions * origin/newstyle: Optimize icons (#943) PropTypes: move fetch-required fields of AppType to be non-requi… (#946) Update ENS API changes from @aragon/wrapper (#944) Permissions: fix debounced search state on clearing filters (#942) Updates for aragonUI and other clean up (#939) AppCenter: update copy (#940)
Behaviour changes:
Chores:
font
andtheme
totextStyles
anduseTheme
.@owisixseven We may want to design a nicer looking screen for when the user uses a wrong URL: