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

Tom/update state docs #333

Merged
merged 5 commits into from
Dec 8, 2023
Merged

Tom/update state docs #333

merged 5 commits into from
Dec 8, 2023

Conversation

tgberkeley
Copy link
Collaborator

Added new var operations page and gave examples of all of the different var operations in use.

@tgberkeley tgberkeley requested review from masenf and Alek99 December 6, 2023 23:10
@tgberkeley
Copy link
Collaborator Author

One more thing I think we could add is more examples of comparing var operations on the front-end to doing the same thing with a computed var on the backend and when we want to do either approach

self.number_1 = random.randint(-10, 10)
self.number_2 = random.randint(-10, 10)

def var_comparison_example():
Copy link
Member

Choose a reason for hiding this comment

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

Lets make this display into a table for a clean visual look

@tgberkeley
Copy link
Collaborator Author

turn all the examples into tables and add badges of green and red for true and false

@Alek99 Alek99 self-requested a review December 7, 2023 20:37
Copy link
Member

@Alek99 Alek99 left a comment

Choose a reason for hiding this comment

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

Nice left a few comments about how we can make the examples more concise

return rx.vstack(
rx.heading(f"The number: {VarOperationState.number}", size="lg"),

rx.heading(f"The negated number is {-VarOperationState.number}", size="md"),
Copy link
Member

Choose a reason for hiding this comment

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

Can we make this more aesthetic and concise?

How about just showing the number at the top, with an hstack below of badges that are short ex Negated: -89, Absolute Val: 89, Len: 4

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done, but I feel the numbers look a little small now

headers=["Integer 1", "Integer 2", "Operation", "Outcome"],
rows=[
(VarComparisonState.number_1, VarComparisonState.number_2, "are equal (==)", f"{VarComparisonState.number_1 == VarComparisonState.number_2}"),
(VarComparisonState.number_1, VarComparisonState.number_2, "are not equal (!=)", f"{VarComparisonState.number_1 != VarComparisonState.number_2}"),
Copy link
Member

Choose a reason for hiding this comment

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

I like the table but I think the operations section doesn't need the explanation as it makes it hard to consume.
Since all the operations are in Python I think we should just do the operation without the description

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

```python exec
import random

class VarComparisonState(State):
Copy link
Member

Choose a reason for hiding this comment

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

Let's call the States something shorted as it will reduce the code significantly in the examples VarComparisonState -> CompState same with the rest of the states in this file

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

rx.table(
headers=["Integer 1", "Integer 2", "Operation", "Outcome"],
rows=[
(VarDivState.number_1, VarDivState.number_2, "True Division of Int 1 by Int 2 (/)", f"{VarDivState.number_1 / VarDivState.number_2}"),
Copy link
Member

Choose a reason for hiding this comment

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

Same here about not needing the comment, as these are regular Python operations

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

rx.heading(f"List 2: {VarListState.list_2}", size="lg"),
rx.heading(f"List 3: {VarListState.list_3}", size="lg"),

rx.heading(f"Does List 1 contain the number 3: {VarListState.list_1.contains(3)}", size="md"),
Copy link
Member

Choose a reason for hiding this comment

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

I think a clean way to display this section would be to have an

hstack(
vstack(
rx.heading(f"List 1: {VarListState.list_1}", size="lg"),
rx.heading(f"Contains 3: {VarListState.list_1.contains(3)}", size="md"),
),
vstack(
rx.heading(f"List 2: {VarListState.list_2}", size="lg"),
rx.heading(f"Reverse List 2: ...),
)
...
)

To better show the correlated parts.

Can shorten these comments
Does List 1 contain the number 3: true -> List 1 Contains 3: True
Join List 3 into string: python -> List 3 Joins: python

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

rx.heading(f"List 1: {VarStringState.string_1}", size="lg"),
rx.heading(f"List 2: {VarStringState.string_2}", size="lg"),

rx.heading(f"Lower Case output of List 1: {VarStringState.string_1.lower()}", size="md"),
Copy link
Member

Choose a reason for hiding this comment

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

Same comment about how to strucutre this section with hstack and vstacks as Contains, Reverse and Join section,

Can shorten the comments up to

List 1 Lower Case: python is fun
List 2 Upper Case: REACT IS HARD

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@Alek99
Copy link
Member

Alek99 commented Dec 8, 2023

Nice much clearer now 👍

@Alek99 Alek99 self-requested a review December 8, 2023 06:42
@Alek99 Alek99 merged commit bcc4ad0 into main Dec 8, 2023
1 check passed
@Alek99 Alek99 deleted the tom/update-state-docs branch December 20, 2023 04:07
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