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

New frontend web client written using Rust and Yew.rs #459

Merged
merged 13 commits into from
Jun 1, 2024

Conversation

chris-dietz
Copy link
Collaborator

@chris-dietz chris-dietz commented May 7, 2024

Adds a new web client runner built from the ground up using rust and yew.rs.
Part of #261

Right now web client must be manually built and launched separate to launching the backend. A followup PR will add a new flag to tell runner to use the new web client. Both the old and new client will be supported until the new front end has been thoroughly tested and has feature parity.

@chris-dietz chris-dietz requested a review from ia0 as a code owner May 7, 2024 21:51
Copy link
Member

@ia0 ia0 left a comment

Choose a reason for hiding this comment

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

Thanks! I didn't review everything yet because this is quite big. Also I have a high-level concern which is that Yew seems to force putting the state of a component in its parent through the notion of properties. I find this quite weird, but maybe that's how it's supposed to be? It seems that implementing Component manually would fix this because we have control over the Message associated type. But then, maybe this will make other things complicated. I would be curious to know if you tried implementing Component explicitly, such that each component has its own state and the properties are only used for configuration by the parent, while the state is modified through message passing. This seems to better match the fact that the app is driven by a websocket which is about message passing.

crates/runner-host/crates/web-client/.gitignore Outdated Show resolved Hide resolved
crates/runner-host/crates/web-client/Cargo.toml Outdated Show resolved Hide resolved
crates/runner-host/crates/web-client/README.md Outdated Show resolved Hide resolved
crates/runner-host/crates/web-client/src/app.rs Outdated Show resolved Hide resolved
crates/runner-host/crates/web-client/src/board.rs Outdated Show resolved Hide resolved
crates/runner-host/crates/web-client/src/board.rs Outdated Show resolved Hide resolved
crates/runner-host/crates/web-common/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Member

@ia0 ia0 left a comment

Choose a reason for hiding this comment

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

Thanks! I think the overall design is there. Now it's mostly about style (except for the console input which I think should be removed).

crates/runner-host/crates/web-client/Cargo.toml Outdated Show resolved Hide resolved
crates/runner-host/crates/web-client/Cargo.toml Outdated Show resolved Hide resolved
crates/runner-host/crates/web-client/Cargo.toml Outdated Show resolved Hide resolved
crates/runner-host/crates/web-client/Cargo.toml Outdated Show resolved Hide resolved
crates/runner-host/crates/web-client/Cargo.toml Outdated Show resolved Hide resolved
crates/runner-host/crates/web-client/src/main.rs Outdated Show resolved Hide resolved
crates/runner-host/crates/web-client/test.sh Outdated Show resolved Hide resolved
crates/runner-host/crates/web-common/src/lib.rs Outdated Show resolved Hide resolved
crates/runner-host/crates/web-client/src/board.rs Outdated Show resolved Hide resolved
Copy link
Member

@ia0 ia0 left a comment

Choose a reason for hiding this comment

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

Thanks! This looks much better now. There's still quite a lot of clones and weird things but I guess that's how Yew is supposed to work. And we could always improve later as we learn Yew better.

crates/runner-host/crates/web-client/src/app.rs Outdated Show resolved Hide resolved
crates/runner-host/crates/web-client/src/app.rs Outdated Show resolved Hide resolved
crates/runner-host/crates/web-client/src/app.rs Outdated Show resolved Hide resolved
crates/runner-host/crates/web-client/src/app.rs Outdated Show resolved Hide resolved
crates/runner-host/crates/web-client/src/board.rs Outdated Show resolved Hide resolved
crates/runner-host/crates/web-client/src/console.rs Outdated Show resolved Hide resolved
crates/runner-host/crates/web-client/src/console.rs Outdated Show resolved Hide resolved
crates/runner-host/crates/web-common/src/lib.rs Outdated Show resolved Hide resolved
@ia0 ia0 added for:maintainability Improves maintainers life runner:host Modifies the Linux runner labels May 25, 2024
@ia0 ia0 changed the base branch from main to dev/web-ui May 29, 2024 09:49
Copy link
Member

@ia0 ia0 left a comment

Choose a reason for hiding this comment

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

I guess this is not yet ready for review.

I've created a dev/web-ui branch (so that we can merge intermediate PRs) and fixed the test.sh which format changed since last main.

crates/runner-host/crates/web-client/test.sh Outdated Show resolved Hide resolved
@ia0
Copy link
Member

ia0 commented May 31, 2024

I guess you are working incrementally or did not push your changes. Feel free to click the "Re-request review" button once the PR is ready for review. Thanks!

@chris-dietz
Copy link
Collaborator Author

I guess you are working incrementally or did not push your changes. Feel free to click the "Re-request review" button once the PR is ready for review. Thanks!

Will do. I think the confusion is I seemed to have messed something up and lost some changes that I made and I hadn't noticed. Something with committing with the web UI and then making and pushing local changes resulted in me losing some work. I think I have redone all of my changes now.

I'm going to do another pass and then I'll request a review again. Sorry about that.

…ckets.

For the moment just sends a fake board ready event so server will send messages as the board is not yet implemented.
Connected board to websocket connection and added button presses and led light up

Connected board to websocket connection

Also added button presses and led light up

Connected board to websocket connection

Also added button presses and led light up

Connected board to websocket connection

Also added button presses and led light up
Changed to match statement

Fixed copyright notice

Fixed gitignore to ignore dist dir as well

Changed log to use inline style

Changed from use_state_eq to use_state

Fixed use effect closures to be more idomatic rust.

Removed unused imports

Moved static assets to data directory

Added test.sh

Incorprated clippy suggestions

Reorded fields

Co-authored-by: Julien Cretin <github@ia0.eu>

Reduced nesting in light conditional

Reduced scopes in closure

Co-authored-by: Julien Cretin <github@ia0.eu>

Reordered use and mod

Co-authored-by: Julien Cretin <github@ia0.eu>

Remove scope and put to clone inline

Co-authored-by: Julien Cretin <github@ia0.eu>

Fixed HTML formatting

Fixed html formatting
@chris-dietz chris-dietz force-pushed the web-client-rs branch 3 times, most recently from d395dd7 to 3cb5c01 Compare June 1, 2024 00:19
chris-dietz and others added 4 commits May 31, 2024 17:25
Co-authored-by: Julien Cretin <github@ia0.eu>
Co-authored-by: Julien Cretin <github@ia0.eu>
Changed to self closing styled elements
@chris-dietz
Copy link
Collaborator Author

Alright I think I have incorporated all the feedback and I also went back and cleaned up the commit history. So I think it should be ready to be re-reviewed now.

Sorry for any confusion. I'll do a better job of waiting to push until I'm ready for things to be reviewed in the future.

Copy link
Member

@ia0 ia0 left a comment

Choose a reason for hiding this comment

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

Thanks! I'll apply the suggestions and merge. Then I'll merge main into dev/web-ui, so you'll have to pull dev/web-ui and work on that branch. In particular, now that we have a branch for this work, you can remove the automatic opening of the old UI and do any change convenient to you until we can replace the old UI, at which point we can merge this branch into main.

@ia0
Copy link
Member

ia0 commented Jun 1, 2024

I'll apply the suggestions and merge.

I spoke too fast. Looks like this PR doesn't have "allow edits by maintainers", so I can't. I'll wait until you apply the suggestions then.

@ia0 ia0 removed for:maintainability Improves maintainers life runner:host Modifies the Linux runner labels Jun 1, 2024
@chris-dietz
Copy link
Collaborator Author

I'll apply the suggestions and merge.

I spoke too fast. Looks like this PR doesn't have "allow edits by maintainers", so I can't. I'll wait until you apply the suggestions then.

I applied the suggested changes. So should be good to merge I think. :)

Co-authored-by: Julien Cretin <github@ia0.eu>
Copy link
Member

@ia0 ia0 left a comment

Choose a reason for hiding this comment

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

Thanks!

@ia0 ia0 merged commit da32659 into google:dev/web-ui Jun 1, 2024
18 checks passed
@ia0
Copy link
Member

ia0 commented Jun 1, 2024

I've merge main into dev/web-ui. You'll need to continue working from that branch.

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