Skip to content

Conversation

@sfc-gh-tszerszen
Copy link
Contributor

📚 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.

    • Bugfix
    • Feature
    • Refactoring
    • Other, please describe:

🧠 Description of Changes

  • Add bullet points summarizing your changes here

    • This is a breaking API change
    • This is a visible (user-facing) change

Revised:

Insert screenshot of your updated UI/code here

Current:

Insert screenshot of existing UI/code here

🧪 Testing Done

  • Screenshots included
  • Added/Updated unit tests
  • Added/Updated e2e tests

🌐 References

Does this depend on other work, documents, or tickets?

  • Issue: Closes #XXXX

Contribution License Agreement

By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.

@sfc-gh-tszerszen sfc-gh-tszerszen added the security-assessment-completed Security assessment has been completed for PR label Mar 3, 2023
@sfc-gh-tszerszen sfc-gh-tszerszen self-assigned this Mar 3, 2023
@sfc-gh-tszerszen sfc-gh-tszerszen force-pushed the improve-deploy-button branch 8 times, most recently from 42f2fff to 37ed756 Compare March 8, 2023 13:23
Copy link
Contributor

@sfc-gh-mnowotka sfc-gh-mnowotka left a 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?

Copy link
Contributor

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?

Copy link
Contributor Author

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
    )
  }

👍

Copy link
Contributor

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.

Copy link
Contributor Author

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 👍

Copy link
Contributor

@sfc-gh-kbregula sfc-gh-kbregula Mar 9, 2023

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?

Copy link
Contributor Author

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

@sfc-gh-tszerszen sfc-gh-tszerszen force-pushed the improve-deploy-button branch 2 times, most recently from 651e817 to 8570abd Compare March 9, 2023 13:58
@sfc-gh-tszerszen sfc-gh-tszerszen force-pushed the improve-deploy-button branch 2 times, most recently from 311fc98 to a39b777 Compare March 22, 2023 09:34
@sfc-gh-tszerszen sfc-gh-tszerszen merged commit f600d83 into develop Mar 23, 2023
tconkling added a commit to tconkling/streamlit that referenced this pull request Mar 27, 2023
* 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)
@sfc-gh-kmcgrady sfc-gh-kmcgrady deleted the improve-deploy-button branch October 5, 2023 19:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

security-assessment-completed Security assessment has been completed for PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants