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

ProgressBar Widget Port #133

Closed
wants to merge 24 commits into from
Closed

Conversation

giannissc
Copy link
Contributor

No description provided.

@giannissc giannissc changed the title Progress bar Port ProgressBar Widget Port Aug 28, 2023
_message: Box<dyn Any>,
_app_state: &mut T,
) -> MessageResult<A> {
MessageResult::Nop
Copy link
Contributor

Choose a reason for hiding this comment

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

Although I agree with Raph, that it should be a logic error when a message reaches this, Stale may be more appropriate here (it's also used in other views/widgets that don't emit messages/events)? Maybe a new MessageResult::Error variant makes sense?
I could also imagine a panic here in debug builds at least.

@giannissc
Copy link
Contributor Author

Should this be merged? I rebased on top of the master @Philipp-M

@Philipp-M
Copy link
Contributor

Haven't had yet time to review the progress bar in detail, it looks like this contains the Switch PR (#132) as well?

I think it would be easier to split the Switch into its own PR (any reason why #132 was closed?).

I can quickly approve #132 (or similar) (would be cool to address the remaining review comments, but they're just nit-picks, so not that important).

@giannissc
Copy link
Contributor Author

I am not sure what happened there.. I thought it got submitted and that why I couldn't find it. I will revive it and rebase both and address the comments so that they can both be submitted.

@Philipp-M
Copy link
Contributor

I think, since you have used the main branch for that PR, and you have synced with the current linebender main branch, that since there weren't any changes in the PR anymore, that the PR has maybe closed itself? I'm not sure though, it hasn't happened to me yet (but I'm not sure if I ever had such a case).

@xorgy xorgy marked this pull request as draft February 8, 2024 18:22
@PoignardAzur
Copy link
Contributor

This PR was made for Xilem classic and is now superseded by #513.

I'm closing this.

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.

3 participants