-
Notifications
You must be signed in to change notification settings - Fork 4k
Improve deploy button #6223
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
Improve deploy button #6223
Conversation
42f2fff to
37ed756
Compare
sfc-gh-mnowotka
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left one small comment in code.
I have a bigger meta-comment though. How do we track usage of this button? Once we introduce it along the modal, what do we expect to change in user behaviour and how are we going to measure it?
frontend/src/App.tsx
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd encapsulate this.props.hostCommunication.currentState.menuItems.length into some function with a meaningful name. Maybe something like isInCloudEnvironment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you Michał, I've introduced:
isInCloudEnvironment = (): boolean => {
const { menuItems } = this.props.hostCommunication.currentState
return menuItems && menuItems?.length > 0
}
showDeployButton = (): boolean => {
if (isTesting()) {
return true
}
return (
isLocalhost() &&
!this.isInCloudEnvironment() &&
SessionInfo.isSet() &&
!SessionInfo.isHello
)
}
👍
frontend/src/App.tsx
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In PR: #6174 I want to limit the use of the isLocalhost function, because some people want to have this behavior also on a remote server, e.g. Docker.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One of us has to merge first and second one adjust, for now whoever gets first 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have any snapshot tests for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've just added some E2E tests for this, please check it out
651e817 to
8570abd
Compare
311fc98 to
a39b777
Compare
ae70975 to
d3d4898
Compare
* develop: StreamlitEndpoints.buildMediaURL (streamlit#6366) Set a default for RuntimeConfig.cache_storage_manager (streamlit#6361) FullScreenWrapper: add a type declaration for react context usage (streamlit#6364) Improve st.help (and st.write's usage of st.help!) (streamlit#5857) Fix regression with query_params (streamlit#6348) Have util.calc_md5 also take bytes (streamlit#6358) Improve deploy button (streamlit#6223) Return whether a secrets.toml file is successfully parsed (streamlit#6333) Add `.webp` to list of safe static file extensions (streamlit#6331) AppContext docstrings (streamlit#6353) fix: upgrade command-line-args from 5.0.2 to 5.2.1 (streamlit#6258) fix: upgrade flatbuffers from 1.11.0 to 1.12.0 (streamlit#6259) sendMessageToHost: no longer a global function (streamlit#6345) Tweak no-else-return config options (streamlit#6343) Allow users to set a secrets.toml file in their home directory (streamlit#6230) Add support for number and boolean types in categorical columns (streamlit#6248) Add support for period type in st.table and st.dataframe (streamlit#5429) Add ability to turn off anchors (streamlit#6158) Add sqlalchemy mypy types to test-requirements.txt (streamlit#6329)
📚 Context
Please describe the project or issue background here
What kind of change does this PR introduce?
This PR introduces Deploy button outside hamburger menu and introduces new Deployment dialog.
🧠 Description of Changes
Add bullet points summarizing your changes here
Revised:
Insert screenshot of your updated UI/code here
Current:
Insert screenshot of existing UI/code here
🧪 Testing Done
🌐 References
Does this depend on other work, documents, or tickets?
Contribution License Agreement
By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.