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

Stability, hide comments and not ready component #41

Merged

Conversation

GerardPaligot
Copy link
Collaborator

@GerardPaligot GerardPaligot commented Jul 15, 2024

Changelog

  • feat: ensure compose and models are stable for Compose Compiler.
  • feat: be able to hide comments.
  • feat: display a component if the feedback is not ready for review.
Capture d’écran 2024-07-15 à 23 36 11 Capture d’écran 2024-07-15 à 23 36 40

@martinbonnin
Copy link
Collaborator

Very cool! Were you able to measure the gain of switching to ImmutableList and friends?

Nit: on the "not ready component", I was tempted to click the smileys. Maybe use something that conveys "loading" instead? (🔜, ⏳,...)

@GerardPaligot
Copy link
Collaborator Author

GerardPaligot commented Jul 16, 2024

Very cool! Were you able to measure the gain of switching to ImmutableList and friends?

The process to measure this kind of thing is very manual. You need to start the layout inspector and count the number of recomposition after every event (didn't find any other way to simplify this kind of thing).

However, for an unknown reason, I wasn't able to inspect the sample app with the layout inspector this time. 🤔

But report files generated by the command in the documentation is very clear about unstable fields and the impact is well documented in the compose documentation. It is probably not a huge impact because the component is pretty small but I prefer to be clean on the library side for every consumer.

Nit: on the "not ready component", I was tempted to click the smileys. Maybe use something that conveys "loading" instead? (🔜, ⏳,...)

Oh, well seen. What about using a color with a lower contrast for the text and icons?

@martinbonnin
Copy link
Collaborator

It is probably not a huge impact because the component is pretty small but I prefer to be clean on the library side for every consumer.

👌

What about using a color with a lower contrast for the text and icons?

Sure, sounds good to me. Take all of this with a grain of salt. As with everything UI related, different people have different expectations so the call is yours ultimately.

@GerardPaligot GerardPaligot force-pushed the feat/stability-comments-notready branch from f808c27 to ef271c1 Compare July 16, 2024 18:05
@GerardPaligot GerardPaligot merged commit 4430322 into paug:main Jul 16, 2024
1 check passed
@GerardPaligot GerardPaligot deleted the feat/stability-comments-notready branch July 16, 2024 18:53
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.

2 participants