-
Notifications
You must be signed in to change notification settings - Fork 7
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
Reskin
| DAO info cards
#1581
Reskin
| DAO info cards
#1581
Conversation
…onditional rendering more apparent
import { t } from 'i18next'; | ||
|
||
interface Props extends ButtonProps { | ||
snapshotURL: string; | ||
} | ||
|
||
export default function Snapshot({ snapshotURL, mt }: Props) { | ||
const SnapshotIconSVG = () => ( |
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 svg can probably instead exist as a replacement to the svg image file I deleted. I tried this because I was not able to modify the icon colour on the <Image>
, but I think it's a similar story with this in-line method. Thoughts?
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.
Ah this definitely can work but Chakra-UI has a createIcon
function that creates a Icon
component. This is how all the current Icons coming from the design package are made currently. Check out: https://github.com/decentdao/decent-ui/blob/develop/design/atoms/components/icons/src/FractalBrand.tsx
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.
Ah, that's good to know. I'm gonna leave as is in this case, since it's like 90% similar and there's no argument for one over the other (in this case).
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.
Looking good, few suggestions and comments
Note: Haven't visually QA yet
import { t } from 'i18next'; | ||
|
||
interface Props extends ButtonProps { | ||
snapshotURL: string; | ||
} | ||
|
||
export default function Snapshot({ snapshotURL, mt }: Props) { | ||
const SnapshotIconSVG = () => ( |
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.
Ah this definitely can work but Chakra-UI has a createIcon
function that creates a Icon
component. This is how all the current Icons coming from the design package are made currently. Check out: https://github.com/decentdao/decent-ui/blob/develop/design/atoms/components/icons/src/FractalBrand.tsx
❌ Deploy Preview for fractal-dev failed. Why did it fail? →
|
Shoot I did not mean to just delete @Da-Colon 's comment. Gonna put it back, I remember what it was about. |
@@ -77,7 +57,6 @@ export default function OptionsList({ | |||
{showOptionCount && ( | |||
<Text | |||
textStyle="text-base-mono-medium" |
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.
Originally a comment from @Da-Colon, which I accidentally deleted:
Don't forget to remove this old
textStyle
(or something like that).
Then, @DarksightKellar, you "resolved" this comment without doing anything about it.
Do we need to remove 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.
Yes it should be removed. Dunno how that happened tbh. Maybe I accidentally reverted while I was staging. Thanks for catching this!
I'm actually thinking of raising a separate PR to remove ALL (non-reskin) textStyles across the app.
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.
Leaving an approval because we just need to play fast and get big bulks of code merged quickly, then worry about the details later.
Addresses #1562 + some refactoring where it made sense to me
I tried not to go wild on the refactoring and it's not a lot tbh, but feel free to push back on any of these updates if they seem problematic to you. I'll push back on the pushback if unconvinced though so full disclosure lol.
I'm gonna try and add comments to this PR on some of the refactor-specific diffs.
Screenshots
Pathetic menu reskin that needs more work. In a separate PR: