Feature/input number #416
Feature/input number #416
Conversation
|
I see a cypress snapshot for darwin but one for linux is missing |
|
Did you add the darwin snapshots by using EDIT: Actually it looks like the darwin snapshots are now ignored by |
lib/streamlit/DeltaGenerator.py
Outdated
| value = 0 | ||
|
|
||
| value = 0 if isinstance(value, NoValue) else value | ||
| int_value = isinstance(value, int) |
There was a problem hiding this comment.
maybe this should be called is_int? same for float_value below
EDIT: I see you have an imported function called is_int so maybe is_int_value instead? Just a suggestion. int_value is fine too but sounds like it's the actual value instead of a boolean
There was a problem hiding this comment.
I changed is_int to is_int_value, because it's checking if a given value is actually an int, here it's being used with an string which contains a number.
lib/streamlit/DeltaGenerator.py
Outdated
| value = 0 | ||
|
|
||
| value = 0 if isinstance(value, NoValue) else value | ||
| int_value = isinstance(value, int) |
There was a problem hiding this comment.
why is it ok to use isinstance here but you use the is_int helper below?
There was a problem hiding this comment.
is_int helper was a later change needed when i checking an str , i will change it
lib/streamlit/DeltaGenerator.py
Outdated
| ---------- | ||
| label : str or None | ||
| A short label explaining to the user what this input is for. | ||
| value : int/float or None |
There was a problem hiding this comment.
it says int/float or None but we throw an error if they pass None I think
There was a problem hiding this comment.
That's something tricky because we could call the function with no value which would be None but if the user passes None we need to raise an exception
lib/streamlit/DeltaGenerator.py
Outdated
| ---------- | ||
| label : str or None | ||
| A short label explaining to the user what this input is for. | ||
| value : int/float or None |
There was a problem hiding this comment.
Not sure where our previous converation went:
@jrhone it says int/float or None but we throw an error if they pass None I think
@arraydude That's something tricky because we could call the function with no value which would be None but if the user passes None we need to raise an exception
If we call the function without passing in a value it gets set to NoValue, not None, right?
There was a problem hiding this comment.
That's right , i'm using that to check when the user specifies None to raise the exception.
tvst
left a comment
There was a problem hiding this comment.
Looks great!!
I updated the CSS in the PR a bit to better match our theme: https://github.com/streamlit/streamlit/pull/416/files/a95c43b827cec72929fb43c883f9dc4905a79863..0f952fc1a8638d8b9a9f8109998b2cf6f3608d8b
One more thing, though: when I double-click the + or - button the "Value" text in the st.write below it gets selected for some reason. Any idea what's up there?
I mean this:
… feature/input-number # Conflicts: # frontend/yarn.lock
tvst
left a comment
There was a problem hiding this comment.
Approved, but please address comments first.
| </span> | ||
| <span | ||
| </button> | ||
| <button |
There was a problem hiding this comment.
This is not a requirement for this PR, just an idea: why not use a BaseWeb Button here?
This way the button's "focus" and "hover" UX state would better match the UX we already have for other BaseWeb Buttons.
Although we'd have to update this button's theme, since our BaseWeb buttons look different from these. (White instead of gray, etc)
If this is too much work, no need to do it. But please at least copy the focus state from our BaseWeb widgets.
There was a problem hiding this comment.
I've changed the focus to get the same style as the hover, i changed the span to button due to the doble click select problem, the problem is if we use the BaseWeb Buttons, we are gonna need to overried the styles to match the feeling of the input as we are doing with the borders and background color, that's why i did not use the baseweb component
| orbit-camera-controller "^4.0.0" | ||
| turntable-camera-controller "^3.0.0" | ||
|
|
||
| "@babel/code-frame@7.5.5", "@babel/code-frame@^7.0.0", "@babel/code-frame@^7.5.5": |
There was a problem hiding this comment.
Uh-oh yarn.lock has changed, so please run e2e tests and look at the screenshots for weirdness! See #479
|
Please add |
|
@tvst After checking the snapshots and added the needed documentation i will merge this PR |
| @@ -0,0 +1,6 @@ | |||
| { | |||
There was a problem hiding this comment.
Did you yarn add from the project root instead of from 'frontend`?
* develop: Update Pipfile (streamlit#505) Release 0.49.0 (streamlit#509) Removing package json (streamlit#501) Feature/input number (streamlit#416)
* develop: Use custom context manager for temporary file in cli.py (streamlit#489) Update pull_request_template.md Update pull_request_template.md Handle lack of trailing slash (streamlit#528) Set auto_envvar_prefix to STREAMLIT (streamlit#477) Strip slashes in path components (streamlit#523) Create doc_improvement.md ESlint: allow @ts-ignore (streamlit#499) Update Pipfile (streamlit#505) Release 0.49.0 (streamlit#509) Removing package json (streamlit#501) Feature/input number (streamlit#416) ESLint warnings are fatal in Circle (streamlit#492) Doc updates (streamlit#286)
Issue: streamlit/streamlit-old-private#767
Description:
Contribution License Agreement
By submiting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.