Add Iframe resizing ability #5894
Conversation
* Made sure iframe resizer is loaded when only embedded * Make block container not grow when embedded * Make footer removal more explicit when embedded * Some clean up of tests
ff3141f to
cc51e48
Compare
|
@kmcgrady I didn't look at the implementation details too much but I tested it in several environments (Hugging Face Spaces, embed mode in Streamlit blog, etc.) and it works like a charm ! |
|
Hey @cbensimon ! Just keeping you updated. I got some tests in and I think it's about ready for the code review. We'll see if it makes our CI happy. The cutoff for the next release is supposed to be Sunday night, so we might get this in time. That being said, I want to make sure it's quality and we tested all scenarios. If not, the next release should be about end of the month. Feel free to test again. I made some tweaks but mostly cosmetic. |
tconkling
left a comment
There was a problem hiding this comment.
Looks good - most of my feedback is just asking for little explanatory comments in places where I felt a bit lost.
One other thing: the syntax_highlighting snapshot changes - are those intentionally part of this PR? Why did they change?
| export const StyledAppViewBlockSpacer = styled.div(({ theme }) => { | ||
| return { | ||
| width: theme.sizes.full, | ||
| flexGrow: 1, | ||
| } | ||
| }) |
There was a problem hiding this comment.
It looks like we don't really add docstrings for these little styled visual elements, but could we start? Like: what does this element do, and where/why do we use it?
There was a problem hiding this comment.
I'm happy to though convention/nomenclature explains this. Let me know if you feel it's worth explicitly saying.
- Spacer is a common term for just an element for the sole purpose to take up space.
- convention shows that the use of this is to provide a spacer in the AppView. Perhaps it makes sense for the comment to be in there to see why this Spacer is included here?
There was a problem hiding this comment.
I assumed this was an AppView-specific element - a "block spacer that we only use in AppView". But if it's just a generic spacer that we'd use in a bunch of places, should it instead live in the top-level styled-components (as just StyledBlockSpacer)?
There was a problem hiding this comment.
I think there's a reason to look into writing up some generic styled components, but this might not be the best example to put there at the moment. In a way, these are spacers that fit in the flexbox framework where other spacers might require different layout engines, which adds more complexity for this case.
|
To answer the question, yes, the |
📚 Context
What kind of change does this PR introduce?
🧠 Description of Changes
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.