Skip to content

Conversation

@tconkling
Copy link
Contributor

@tconkling tconkling commented Mar 30, 2021

An st_form refactor that introduces the ImmerJS library, and lays the groundwork for the upcoming "warn on missing submit button" PR

  • FormsData is now an interface rather than a class
  • FormsManager updates FormsData via immer.produce
  • Form is now its own class, and has its own styled-components.ts

- FormsData is now an interface rather than a class
- FormsManager updates FormsData via immer.produce
- Form is now its own class, and has its own styled-components.ts
@tconkling tconkling requested review from a team and kantuni March 30, 2021 22:19
Comment on lines +25 to +33
export class Form extends PureComponent<Props> {
public render = (): ReactNode => {
return (
<StyledForm data-testid="stForm" width={this.props.width}>
{this.props.children}
</StyledForm>
)
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Form is going to get some new functionality in a future PR, and it was convenient to make it a class in this PR to support that upcoming work.

// sets, maps, and arrays.
formsMgr.setPendingForms(new Set(["one", "two"]))
expect(() => formsData.formsWithPendingChanges.clear()).toThrowError(
"[Immer] This object has been frozen and should not be mutated"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This relies heavily on the implementation of Immer. Meaning, if Immer decides to change the error message tomorrow - this test would fail for "no reason".
Can we test the expected behavior using Object.isFrozen() or some other way?

Copy link
Collaborator

@kantuni kantuni left a comment

Choose a reason for hiding this comment

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

Looks great!

* st_form:
  Added "allow-downloads" to the sandbox attributes (streamlit#3053)
  Small fix for `make pylint` command (streamlit#3062)
  Set genericColors properly and make theme defs more consistent (streamlit#3051)
  Downgrade st.warning in local_sources_watcher to a log (streamlit#3050)
  Extend docstring of st.image (streamlit#3055)
  Move docs.streamlit.io to Segment (streamlit#3005)
  Fix bokeh example in docs (`legend` arg) (streamlit#2907)
  Remove incorrect markdown table styling (streamlit#3038)
  document getattr
  Show the beta_warning only a single time per object
  object_beta_warning: handle every __ magic method
  object_beta_warning: handle the subscript operator
  beta_util_test
  Move secrets out of beta
  [Security] Upgrade y18n to 4.0.1 or later (streamlit#3041)
  Fix gap not working on Safari (streamlit#3042)
  formatting
  call _repr_html_ when available
@tconkling tconkling merged commit 7876b5c into streamlit:st_form Apr 5, 2021
@tconkling tconkling deleted the tim/FormsManager branch April 5, 2021 18:13
tconkling added a commit to tconkling/streamlit that referenced this pull request Apr 5, 2021
# By Tim Conkling (7) and others
# Via GitHub (3) and Tim Conkling (1)
* st_form:
  FormsManager (streamlit#3046)
  Added "allow-downloads" to the sandbox attributes (streamlit#3053)
  Small fix for `make pylint` command (streamlit#3062)
  Set genericColors properly and make theme defs more consistent (streamlit#3051)
  Downgrade st.warning in local_sources_watcher to a log (streamlit#3050)
  Extend docstring of st.image (streamlit#3055)
  Move docs.streamlit.io to Segment (streamlit#3005)
  Fix bokeh example in docs (`legend` arg) (streamlit#2907)
  Remove incorrect markdown table styling (streamlit#3038)
  document getattr
  Show the beta_warning only a single time per object
  object_beta_warning: handle every __ magic method
  object_beta_warning: handle the subscript operator
  beta_util_test
  Move secrets out of beta
  [Security] Upgrade y18n to 4.0.1 or later (streamlit#3041)
  Fix gap not working on Safari (streamlit#3042)
  formatting
  call _repr_html_ when available

# Conflicts:
#	frontend/src/components/core/Block/Block.tsx
#	frontend/src/components/widgets/Form/Form.tsx
#	frontend/src/components/widgets/Form/FormSubmitButton.tsx
#	frontend/src/components/widgets/Form/FormsData.test.ts
#	frontend/src/components/widgets/Form/FormsManager.ts
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