enable scrolling locally, but keep disabled in iframe #43
Conversation
WalkthroughThis update introduces a new control flow in the application root. In Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
🔇 Additional comments (7)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pages/_app.js(1 hunks)styles.css(1 hunks)
🔇 Additional comments (2)
pages/_app.js (1)
14-19: LGTM! Clean component structure maintained.The component structure is clean and the return statement properly renders both the IframeCommunication component and the main Component with its props.
styles.css (1)
7-9: LGTM! CSS implementation aligns with PR objective.The CSS implementation correctly:
- Uses the :not pseudo-class for clean selector logic
- Maintains hidden overflow in iframe context
- Enables scrolling in local context
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
components/iframeCommunication.js (1)
45-45: Consider making the height difference threshold configurable.The 40-pixel threshold for sending height updates seems arbitrary. Consider making it a configurable constant or prop to allow adjustment based on different use cases.
+const HEIGHT_UPDATE_THRESHOLD = 40; // pixels + const IframeCommunication = () => { const router = useRouter(); const lastHeight = useRef(0); const debounceTimeout = useRef(null); const sendPageHeight = () => { const contentHeight = document.body.offsetHeight; if (process.env.NODE_ENV === 'development') { console.log("Actual content height:", contentHeight); } - if (Math.abs(contentHeight - lastHeight.current) > 40) { + if (Math.abs(contentHeight - lastHeight.current) > HEIGHT_UPDATE_THRESHOLD) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/iframeCommunication.js(1 hunks)
🔇 Additional comments (2)
components/iframeCommunication.js (2)
54-54: Formatting change noted.
41-43: Verify height calculation behavior with hidden content.The change from
scrollHeighttooffsetHeightmight not account for content that's hidden due to overflow. Please verify this change doesn't cause issues with:
- Dynamic content loading
- Expandable sections
- Long content that should be scrollable
Additionally, consider wrapping the debug log in a development check:
- console.log("Actual content height:", contentHeight); + if (process.env.NODE_ENV === 'development') { + console.log("Actual content height:", contentHeight); + }
Describe the background of your pull request
What does your pull request do? Does it solve a bug (which one?), add a feature?
Additional context
Add any other context
Governance
By submitting this pull request I confirm that:
https://github.com/alkem-io/Coordination/blob/master/LICENSE.
Summary by CodeRabbit
New Features
Style
Chores
Bug Fixes