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

Add button selector #254

Merged
merged 2 commits into from
Oct 7, 2024
Merged

Conversation

daxmu92
Copy link
Contributor

@daxmu92 daxmu92 commented Oct 7, 2024

No description provided.

Copy link

netlify bot commented Oct 7, 2024

Deploy Preview for phenomenal-crepe-0effec ready!

Name Link
🔨 Latest commit edf090e
🔍 Latest deploy log https://app.netlify.com/sites/phenomenal-crepe-0effec/deploys/6703e3fee4e1670008ec45c7
😎 Deploy Preview https://deploy-preview-254--phenomenal-crepe-0effec.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@daxmu92
Copy link
Contributor Author

daxmu92 commented Oct 7, 2024

@arnaudmiribel This is my first PR to streamlit-extras. Could you help review? Appreciate for any suggestions!

@arnaudmiribel
Copy link
Owner

Hey @daxmu92, this is a lovely contribution! Thanks a lot. Some feedback:

  1. Did you run pre-commit on your branch to make sure the linting is done right? (see the pip install pre-commit && pre-commit run --all should do! Looks like the test is failing, it seems to not be related to you though, I'll have a look.
  2. Can you please update the arguments so they are more consistent with existing arguments found in Streamlit native commands? I think that will be helpful for developers to understand how to use your extra. Let me know what you think. Here are some suggestions:
    CleanShot 2024-10-07 at 11 58 55@2x

@daxmu92
Copy link
Contributor Author

daxmu92 commented Oct 7, 2024

@arnaudmiribel Thanks for you kindly reply! I have fixed most of them and the cicd shows passed.

However, in the generated deploy-preview, it can't find my extra:

ModuleNotFoundError: No module named 'streamlit_extras.button_selector'

Did I miss anything?

@arnaudmiribel
Copy link
Owner

Hey @daxmu92, thank you so much for the changes! There seems to be an issue remaining with pre-commit but that nothing do to with your code, I ran the pre-commit on your branch and it seemed to work fine.

As for the ModuleNotFoundError in the Output (beta) container of the preview docs, it is an expected limitation. In fact, the documentation builds off the branch code (that's why we can see your contribution in there!) but stlite, the technology that's used to render the code snippet, can't build streamlit_extras from the branch source, and instead relies on the latest available version that's on PyPI, which obviously doesn't include your extra, yet! Upon releasing a new version in PyPI, that part should work though!

@daxmu92
Copy link
Contributor Author

daxmu92 commented Oct 7, 2024

Thanks! Let me know if you have other concerns.

@arnaudmiribel
Copy link
Owner

Opened #255 that should be merged (and this branch should pull after that) - which should fix the linting CI failing

@arnaudmiribel
Copy link
Owner

@daxmu92 can you pull from main? Should resolve the issue

Copy link
Owner

@arnaudmiribel arnaudmiribel left a comment

Choose a reason for hiding this comment

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

Yay, nice one @daxmu92!

@arnaudmiribel arnaudmiribel merged commit 41c2ec1 into arnaudmiribel:main Oct 7, 2024
8 checks passed
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