Skip to content

example of function memory game#2255

Merged
ranile merged 5 commits into
yewstack:masterfrom
leftstick:master
Jan 12, 2022
Merged

example of function memory game#2255
ranile merged 5 commits into
yewstack:masterfrom
leftstick:master

Conversation

@leftstick
Copy link
Copy Markdown
Contributor

@leftstick leftstick commented Dec 9, 2021

Description

Example of function memory game.
It is a different case to demonstrate how functional components work.

check below demo:

demo

Checklist

  • I have run cargo make pr-flow
  • I have reviewed my own code

@voidpumpkin
Copy link
Copy Markdown
Member

@leftstick We really appreciate your PR, and I personally love the look of the example.
But sorry we have a lot of examples to maintain already, so currently we only merge examples that highlight distinct features of the library.
Also we already have functional components in the function_todomvc example, sorry.

But you could add a new page in the yew docs for community examples/links. Then add a link to your example repository there!

@siku2
Copy link
Copy Markdown
Member

siku2 commented Dec 11, 2021

I think this example is significantly more interesting than the function_todomvc example.
I know that function_todomvc gains some value by providing a direct comparison to struct components but I honestly don't see that as a big deal. It also adds maintenance overhead since the expectation is that the example is a one to one comparison to the other todomvc example.

I'd much rather incorporate this example and put a few struct to function component comparisons in the documentation.

@voidpumpkin voidpumpkin reopened this Dec 11, 2021
@leftstick
Copy link
Copy Markdown
Contributor Author

@voidpumpkin @siku2 thanks for your suggestion. Since it is the first time I tried rust, the implementation might not elegant enough. Let me know if you have any further comments.

@voidpumpkin voidpumpkin added the A-examples Area: The examples label Dec 15, 2021
Copy link
Copy Markdown
Member

@voidpumpkin voidpumpkin left a comment

Choose a reason for hiding this comment

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

All looks good except all the manual PartialEq implementations.

Comment on lines +7 to +25
#[derive(Properties, Clone)]
pub struct Props {
pub cards: Vec<Card>,
pub on_flip: Callback<RawCard>,
}

impl PartialEq for Props {
fn eq(&self, other: &Props) -> bool {
let s_cards = &self.cards;
let o_cards = &other.cards;
let s_cards_len = s_cards.len();
let o_cards_len = o_cards.len();

match s_cards_len == o_cards_len {
false => false,
true => s_cards.iter().all(|c| o_cards.contains(c)),
}
}
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm a little confused why not derive PartialEq automatically?

Suggested change
#[derive(Properties, Clone)]
pub struct Props {
pub cards: Vec<Card>,
pub on_flip: Callback<RawCard>,
}
impl PartialEq for Props {
fn eq(&self, other: &Props) -> bool {
let s_cards = &self.cards;
let o_cards = &other.cards;
let s_cards_len = s_cards.len();
let o_cards_len = o_cards.len();
match s_cards_len == o_cards_len {
false => false,
true => s_cards.iter().all(|c| o_cards.contains(c)),
}
}
}
#[derive(Properties, Clone, PartialEq)]
pub struct Props {
pub cards: Vec<Card>,
pub on_flip: Callback<RawCard>,
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@voidpumpkin Thanks for your suggestion. I misunderstood the automatic PartialEq before. I tried replace with automatic implementation, it's working properly.
I've committed the change

voidpumpkin
voidpumpkin previously approved these changes Dec 29, 2021
Copy link
Copy Markdown
Member

@ranile ranile left a comment

Choose a reason for hiding this comment

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

Might also want to use the new syntax for function component macro (one where function name is the component name). It's also fine as is

Comment thread examples/function_memory_game/README.md Outdated
Comment on lines +13 to +15
## LICENSE

[MIT License](https://raw.githubusercontent.com/yewstack/yew/examples/function_memory_game/master/LICENSE)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why is the license specified here? Why is it only MIT? The example's Cargo.toml says the license is MIT to Apache 2.0.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@hamza1311 this example should not have a specific license. I will remove this part

@voidpumpkin
Copy link
Copy Markdown
Member

@leftstick also take a look at comments in #2313

I had a copy of your PR before you fixed yours so @siku2 reviewed that one

Copy link
Copy Markdown
Member

@voidpumpkin voidpumpkin left a comment

Choose a reason for hiding this comment

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

Minor comments

Comment thread examples/function_memory_game/src/components/chessboard.rs
Comment thread examples/function_memory_game/src/components/chessboard_card.rs Outdated
Comment thread examples/function_memory_game/src/components/game_status_board.rs Outdated
Comment thread examples/function_memory_game/src/components/game_status_board.rs Outdated
Comment thread examples/function_memory_game/src/components/score_board.rs Outdated
Comment thread examples/function_memory_game/src/components/score_board_best_score.rs Outdated
Comment thread examples/function_memory_game/src/components/score_board_logo.rs Outdated
Comment thread examples/function_memory_game/src/components/score_board_progress.rs Outdated
Comment thread examples/function_memory_game/src/main.rs Outdated
@leftstick
Copy link
Copy Markdown
Contributor Author

@voidpumpkin code changed. Thanks for reviewing

voidpumpkin
voidpumpkin previously approved these changes Jan 6, 2022
@voidpumpkin
Copy link
Copy Markdown
Member

Please rebase onto master for tests to be fixed

@leftstick
Copy link
Copy Markdown
Contributor Author

Please rebase onto master for tests to be fixed

@voidpumpkin done. Thanks for reminding

voidpumpkin
voidpumpkin previously approved these changes Jan 7, 2022
@voidpumpkin voidpumpkin requested a review from ranile January 7, 2022 07:27
Copy link
Copy Markdown
Member

@ranile ranile left a comment

Choose a reason for hiding this comment

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

Just formatting and warnings. Otherwise, looks good


match props.status {
Status::Ready => html! {
<span >{"Ready"}</span>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
<span >{"Ready"}</span>
<span>{"Ready"}</span>

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@hamza1311 Would you please share how you got such error? I ran with cargo make lint, there is no any warning. Thanks

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is not a warning, just a formatting issue. The warnings are in other comment where @voidpumpkin explained how to run the lints

<span >{"Ready"}</span>
},
Status::Playing => html! {
<span >{"Playing"}</span>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
<span >{"Playing"}</span>
<span>{"Playing"}</span>

html! {
<div class="chess-board-card-container">
<div class={classes!("card", flipped.then(|| "flipped"))} {onclick}>
<img class="front" src={get_link_by_cardname} />
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please fix the warnings

Copy link
Copy Markdown
Contributor Author

@leftstick leftstick Jan 11, 2022

Choose a reason for hiding this comment

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

@hamza1311 would you please share how you got those warnings? I got everything alright with cargo make lint. Thanks

Copy link
Copy Markdown
Member

@voidpumpkin voidpumpkin Jan 11, 2022

Choose a reason for hiding this comment

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

@leftstick Those are yew custom warnings that work with only nightly compiler
https://yew.rs/docs/concepts/html/introduction#lints

@voidpumpkin voidpumpkin added the S-waiting-on-author Status: awaiting action from the author of the issue/PR label Jan 10, 2022
@leftstick
Copy link
Copy Markdown
Contributor Author

@voidpumpkin @hamza1311 done. Thanks for reviewing

@voidpumpkin voidpumpkin removed the S-waiting-on-author Status: awaiting action from the author of the issue/PR label Jan 12, 2022
Copy link
Copy Markdown
Member

@ranile ranile left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks for the PR

@ranile ranile merged commit fff1ffa into yewstack:master Jan 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-examples Area: The examples

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants