search

leather-io / mono

Leather monorepo
https://leather.io
MIT License
9 stars 4 forks source link

feat: radio button and theme modal #427

Closed fbwoolf closed 1 week ago

fbwoolf commented 1 week ago

This PR add a RadioButton to the UI library and wires up the settings theme modal. I removed the theme toggle in the dev console, but let me know if people prefer we keep it?

signal-2024-09-04-130502_004 signal-2024-09-04-130502_002

*Are we wanting to default to light or system? Currently, we are defaulting to light.

fbwoolf commented 1 week ago

@kyranjamie @pete-watters can you take a look at my second commit here? I spent quite a bit of time refactoring Cell today. You will see the old one renamed CellToReplace temporarily, I'll swap those out once we all agree my changes are ok. Look at the new Cell and CellWithAvatar (open to feedback on naming). Cell does not use the Flag so covers the last one here, while CellWithAvatar uses Flag to handle the others. I added an actionIcon so we can pass in the Switch or RadioButton as needed (or chevron). This removes needing the variant. Look at ThemeSwitcher for sample implementation.

Screenshot 2024-09-05 at 8 20 37 PM
pete-watters commented 1 week ago

@kyranjamie @pete-watters can you take a look at my second commit here? I spent quite a bit of time refactoring Cell today. You will see the old one renamed CellToReplace temporarily, I'll swap those out once we all agree my changes are ok. Look at the new Cell and CellWithAvatar (open to feedback on naming). Cell does not use the Flag so covers the last one here, while CellWithAvatar uses Flag to handle the others. I added an actionIcon so we can pass in the Switch or RadioButton as needed (or chevron). This removes needing the variant. Look at ThemeSwitcher for sample implementation.

Screenshot 2024-09-05 at 8 20 37 PM

This looks great to me, thanks for taking it on @fbwoolf 👍 It's much cleaner using ItemLayout and worth taking the time now to then update all the Cells.

I just have one Q about removing the flex={1 as thats something I think all views / box's need. We could even just add it to our box.native wrapper though to avoid having to always add it.

For context @kyranjamie , this is the discussion we originally had about it https://github.com/leather-io/mono/pull/416#discussion_r1740683610

kyranjamie commented 1 week ago

Some of these conflicts are because of me, but hopefully just import path ones