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

Composer - replace threadgate modal with alf dialog #4329

Merged
merged 19 commits into from
Jun 24, 2024
Merged

Conversation

mozzius
Copy link
Member

@mozzius mozzius commented Jun 3, 2024

iOS default iOS with selections Web
Simulator Screenshot - iPhone 15 - 2024-06-03 at 12 08 44 Simulator Screenshot - iPhone 15 - 2024-06-03 at 12 08 50 Screenshot 2024-06-03 at 12 15 16

Copy link

render bot commented Jun 3, 2024

Copy link

github-actions bot commented Jun 3, 2024

Old size New size Diff
6.47 MB 6.47 MB -370 B (-0.01%)

@mozzius mozzius force-pushed the samuel/alf-threadgate branch from 5af0c1a to 7eac04b Compare June 4, 2024 10:17
@gaearon
Copy link
Collaborator

gaearon commented Jun 4, 2024

Pressing Escape now tries to dismiss both dialogs (previously it was ignored). We need to fix this to only dismiss the inner one (like with GIF picker).

@gaearon
Copy link
Collaborator

gaearon commented Jun 4, 2024

also getting this weird spinner

Screenshot 2024-06-04 at 11 59 56

@mozzius
Copy link
Member Author

mozzius commented Jun 4, 2024

That's it loading your lists, maybe we should change the layout so that it's less janky

@gaearon
Copy link
Collaborator

gaearon commented Jun 4, 2024

nah, it never loads. just stuck like this. there also no new network requests.

on main, there's no spinner.

Copy link
Collaborator

@gaearon gaearon left a comment

Choose a reason for hiding this comment

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

  • need to fix Escape handling
  • distinguish no lists vs loading lists

@mozzius mozzius requested a review from gaearon June 4, 2024 11:40
@gaearon
Copy link
Collaborator

gaearon commented Jun 4, 2024

The height is jumping when changing selection, probably due to borders appearing and disappearing:

height.mov

It would be good to stabilize those.

Also, I'm not a big fan of using the same color for selection and the primary action:

Screenshot 2024-06-04 at 15 00 05

Let's do a pass over these before getting it in.

@pfrazee
Copy link
Collaborator

pfrazee commented Jun 24, 2024

Updated the buttons

one two
CleanShot 2024-06-24 at 11 34 13@2x CleanShot 2024-06-24 at 11 34 39@2x

@pfrazee
Copy link
Collaborator

pfrazee commented Jun 24, 2024

@gaearon Interesting, is that 2-states solution preferable to using an effect?

@gaearon
Copy link
Collaborator

gaearon commented Jun 24, 2024

yeah, it avoids a "roundtrip", see here https://react.dev/reference/react/useState#storing-information-from-previous-renders:

This pattern can be hard to understand and is usually best avoided. However, it’s better than updating state in an effect. When you call the set function during render, React will re-render that component immediately after your component exits with a return statement, and before rendering the children. This way, children don’t need to render twice. The rest of your component function will still execute (and the result will be thrown away). If your condition is below all the Hook calls, you may add an early return; to restart rendering earlier.

however i think i'll actually move the state down so that it gets torn down on dialog close

Copy link
Collaborator

@pfrazee pfrazee left a comment

Choose a reason for hiding this comment

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

Excellent work @mozzius @gaearon

@gaearon gaearon merged commit 29aaf09 into main Jun 24, 2024
6 checks passed
@pfrazee pfrazee deleted the samuel/alf-threadgate branch June 24, 2024 23:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants