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

Communication Examples Style and Code Clean-up #3180

Merged
merged 21 commits into from Apr 3, 2023

Conversation

Jaffa-Cakes
Copy link
Contributor

@Jaffa-Cakes Jaffa-Cakes commented Mar 24, 2023

Description

This PR aims to improve the styling and code layout of the following examples:

Feel free to provide me with any feedback you have on these changes.

Checklist

  • I have reviewed my own code
  • Improved communication_child_to_parent
  • Improved communication_grandchild_with_grandparent
  • Improved communication_grandparent_to_grandchild
  • Improved communication_parent_to_child

The checklist item for additional tests has been omitted as they are not necessary in this situation.

Reasoning

The examples README.md mentions any help with styling would be appreciated; I thought it would be a good idea to start churning through some examples to make things look nicer. 😃

Before/After of Examples

Child to Parent

Before

Child to Parent - Before

After

Child to Parent - After
Grandparent with Grandchild

Before

Grandchild with Grandparent - Before

After

Grandchild with Grandparent - After
Grandparent to Grandchild

Before

Grandparent to Grandchild -Before

After

Grandparent to Grandchild - After
Parent to Child

Before

Parent to Child - Before

After

Parent to Child - After

@github-actions
Copy link

github-actions bot commented Mar 24, 2023

Size Comparison

examples master (KB) pull request (KB) diff (KB) diff (%)
async_clock 99.081 99.081 0 0.000%
boids 169.249 169.249 0 0.000%
communication_child_to_parent 90.012 91.613 +1.602 +1.779%
communication_grandchild_with_grandparent 103.549 105.700 +2.151 +2.078%
communication_grandparent_to_grandchild 99.569 101.886 +2.316 +2.326%
communication_parent_to_child 87.264 88.642 +1.378 +1.579%
contexts 105.854 105.854 0 0.000%
counter 85.229 85.229 0 0.000%
counter_functional 85.414 85.414 0 0.000%
dyn_create_destroy_apps 88.012 88.012 0 0.000%
file_upload 99.314 99.314 0 0.000%
function_memory_game 162.304 162.304 0 0.000%
function_router 335.514 335.514 0 0.000%
function_todomvc 157.921 157.921 0 0.000%
futures 222.993 222.993 0 0.000%
game_of_life 105.891 105.891 0 0.000%
immutable 179.303 179.303 0 0.000%
inner_html 81.640 81.640 0 0.000%
js_callback 110.185 110.185 0 0.000%
keyed_list 197.180 197.180 0 0.000%
mount_point 84.809 84.809 0 0.000%
nested_list 111.349 111.349 0 0.000%
node_refs 92.479 92.479 0 0.000%
password_strength 1534.787 1534.787 0 0.000%
portals 95.625 95.625 0 0.000%
router 307.044 307.044 0 0.000%
simple_ssr 140.469 140.469 0 0.000%
ssr_router 372.244 372.244 0 0.000%
suspense 107.376 107.376 0 0.000%
timer 88.108 88.108 0 0.000%
todomvc 140.028 140.028 0 0.000%
two_apps 85.832 85.832 0 0.000%
web_worker_fib 149.964 149.964 0 0.000%
webgl 84.349 84.349 0 0.000%

⚠️ The following examples have changed their size significantly:

examples master (KB) pull request (KB) diff (KB) diff (%)
communication_child_to_parent 90.012 91.613 +1.602 +1.779%
communication_grandchild_with_grandparent 103.549 105.700 +2.151 +2.078%
communication_grandparent_to_grandchild 99.569 101.886 +2.316 +2.326%
communication_parent_to_child 87.264 88.642 +1.378 +1.579%

@Jaffa-Cakes Jaffa-Cakes marked this pull request as ready for review March 24, 2023 15:32
@Jaffa-Cakes Jaffa-Cakes changed the title Communication Examples Clean-up Communication Examples Style and Code Clean-up Mar 24, 2023
Copy link

@tobyscott25 tobyscott25 left a comment

Choose a reason for hiding this comment

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

I like the separation of parent and child components into separate modules, as would be done in the real world. But I don't think this is the right place to introduce Tailwind as it bloats the HTML in what needs to be simple component examples.

Don't get me wrong, I love Tailwind, but I think it's probably a better idea to create a separate example on using Tailwind with Yew. Perhaps you could create an assortment of examples for using different css libraries 🙂

P.S. Approved by accident before, apologies.

@Jaffa-Cakes
Copy link
Contributor Author

@tobyscott25:

I don't think this is the right place to introduce Tailwind as it bloats the HTML in what needs to be simple component examples.

I'm on the other side of things in believing it should be the way to do things, my case is that it:

  • Reduces the amount of time spent on styling for the dev making the example.
  • Increases the amount of standardisation as a library like Tailwind has standardised colours, padding/margin sizes, and syntax.
  • Is served through the versioned CDN making it a simple job to drop it into the HTML head tag with minimum worries and have nothing duplicated into the repo from Tailwind.

It would be ideal to have custom classes written in raw CSS, but it would take a lot more time to create and also might end up looking subpar such as how these examples look at the moment without the changes this PR makes.

Overall I believe this to be a worthwhile trade-off - if someone is attempting to use the Yew framework, it would be likely for them to be familiar with CSS libraries such as Tailwind; even if they were not familiar, the extra text this adds to the HTML is not difficult to understand so long as you know what a class is in general, it also does not affect the core demonstration of the example. If the viewer doesn't properly understand HTML structure then I'm guessing they should probably be starting with stuff a bit more basic than Yew.

The time that developers spend on generating these examples and making contributions to Yew should be an important thing to consider, Tailwind offers the best of both worlds in terms of style and fast development.

@tobyscott25
Copy link

Speed of development is important, although I still think the priority of these examples should be simplicity over anything else.

IMO there are fundamentally different priorities between creating examples that prove a basic concept and building an entire project. But I'm not a maintainer of this repo ¯_(ツ)_/¯

@futursolo
Copy link
Member

I would agree with @tobyscott25's opinion that we should keep the examples as simple as possible.
I do not think it's difficult to remake the styling with pure css anyways.

Using local styles also makes it possible to run these examples offline without bundling tailwind.

@Jaffa-Cakes
Copy link
Contributor Author

I've refactored out Tailwind and readded the index.scss files for each example as you both prefer @tobyscott25 and @futursolo, please let me know if there is anything else you would like changed. 👍

Copy link
Member

@futursolo futursolo left a comment

Choose a reason for hiding this comment

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

Thank you for the pull request! Looks good to me.

Copy link

@tobyscott25 tobyscott25 left a comment

Choose a reason for hiding this comment

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

LGTM, nice work

@voidpumpkin voidpumpkin merged commit ce3eeec into yewstack:master Apr 3, 2023
7 of 8 checks passed
@voidpumpkin voidpumpkin added the A-examples Area: The examples label Apr 3, 2023
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.

None yet

4 participants