Also use bottom padding in embedded mode for chat input #6979
Conversation
| className="block-container" | ||
| data-testid="block-container" | ||
| isWideMode={wideMode} | ||
| showPadding={showPadding} |
There was a problem hiding this comment.
why could we not just do:
showPadding={showPadding || containsChatInput} with a comment?
There was a problem hiding this comment.
I think this would also add the header padding, which we don't want in this case. If it is embedded and has a chat input, we only want the bigger bottom padding.
There was a problem hiding this comment.
I think it would then be addPaddingForHeader={(showToolbar || showColoredLine) && !containsChatInput} then?
There was a problem hiding this comment.
hmm, addPaddingForHeader is adding a 3rem top padding instead of 1rem.
It would probably mostly work fine, but it would impact apps like the one embedded on the docs page here (which doesn't have a toolbar / color line)
There was a problem hiding this comment.
I'm also fine with this solution. But I think we anyways need to refactor this a bit on how the "fullscreen" chat input works together with the AppView (-> I would love to move it to some kind of bottom root container). So the addPaddingForChatInput is probably more of a temporary change.
There was a problem hiding this comment.
Alright. I am fine with it. Can we add a screenshot test then?
There was a problem hiding this comment.
This could be a bit tricky since we don't have precedence (as far as I know) for e2e testing embedded apps. But I will take another look into this.
There was a problem hiding this comment.
You can check out #5894 which shows an example of the iframe resizer working.
There was a problem hiding this comment.
ahh, didn't know about this one. Already added a test using http://localhost:3000/?embed=true which seems to work fine for checking the padding.
|
@kmcgrady I added an e2e test. It only checks for the correct padding, but I think this is probably sufficient in this case. |
* develop: Feature: st.toast (streamlit#6783) Bump semver from 5.7.1 to 5.7.2 in /frontend (streamlit#6982) Add host communication e2e tests (streamlit#6806) Re-add homepage to package.json (streamlit#6987) Remove unnecessary code and Add rm commands to make clean (streamlit#6980) Allow setting placeholder for st.selectbox (streamlit#6913) Allow setting placeholder for st.multiselect (streamlit#6901) Also use bottom padding in embedded mode for chat input (streamlit#6979) Feature/st lib (streamlit#6692) Remove unused import (streamlit#6977) Release 1.24.1 (streamlit#6965) Update st.audio/st.video docstrings (streamlit#6964) Slightly simplify bug report template (streamlit#6972) Fix baseweb warnings by using longhand properties (streamlit#6976)
* Also use bottom padding in embedded mode * Change bottom embed padding back to const * Add e2e test to check css * Add license header * Use px instead of rem
Describe your changes
Currently, messages are cut off when
st.chat_inputis used in an embedded app. To fix this, we are always using the10rembottom padding regardless if it is embedded or not if the app is usingchat_input.Note: This is more of a quick fix. We eventually want to refactor how chat input in fullscreen mode interacts with the app view (e.g. introduce a bottom container).
Current state:
After the change:
Contribution License Agreement
By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.