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

fix(Tile): recalculate tile rect height on component did mount #9593

Merged
merged 14 commits into from
Sep 24, 2021

Conversation

dakahn
Copy link
Contributor

@dakahn dakahn commented Sep 3, 2021

Closes #9187

Adds a resize observer to Tile that recalculates the rect height to account for tiles initially rendered as hidden (like in a set of tabs)

@dakahn dakahn requested a review from a team as a code owner September 3, 2021 02:37
@netlify
Copy link

netlify bot commented Sep 3, 2021

✔️ Deploy Preview for carbon-react-next ready!

🔨 Explore the source changes: 2250284

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-react-next/deploys/614ddea67b8b480008df2766

😎 Browse the preview: https://deploy-preview-9593--carbon-react-next.netlify.app

@netlify
Copy link

netlify bot commented Sep 3, 2021

✔️ Deploy Preview for carbon-elements ready!

🔨 Explore the source changes: 2250284

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-elements/deploys/614ddea6a2c3040007cf051b

😎 Browse the preview: https://deploy-preview-9593--carbon-elements.netlify.app

@netlify
Copy link

netlify bot commented Sep 3, 2021

✔️ Deploy Preview for carbon-components-react ready!
Built without sensitive environment variables

🔨 Explore the source changes: 2250284

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-components-react/deploys/614ddea6ca1aac00087e1a1c

😎 Browse the preview: https://deploy-preview-9593--carbon-components-react.netlify.app

Copy link
Member

@sstrubberg sstrubberg left a comment

Choose a reason for hiding this comment

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

Getting a CORS error in deploy preview.

image

This reminds me exactly of a PR i just recently spent way too much time debugging. I think it's Storybook acting up. I ended up removing the Storybook Actions Add-on for Button so it would work.

Also, looks like tests are failing.

Copy link
Contributor

@joshblack joshblack left a comment

Choose a reason for hiding this comment

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

Just a small note with the id change.

A question that I don't know if we considered, why do we need to calculate the height of the "above the fold" content in the first place? Can't we just only render the above the fold content until the tile is expanded and then render the rest of the content? 🤔

@dakahn
Copy link
Contributor Author

dakahn commented Sep 7, 2021

Just a small note with the id change.

A question that I don't know if we considered, why do we need to calculate the height of the "above the fold" content in the first place? Can't we just only render the above the fold content until the tile is expanded and then render the rest of the content?

That makes sense to me @joshblack -- should I open a ticket for a minor tile refactor?

@joshblack
Copy link
Contributor

@dakahn sounds great to me!

@dakahn
Copy link
Contributor Author

dakahn commented Sep 8, 2021

@joshblack wrapped the resizeObserver work in before/after each blocks and scoped it to the expandable tile tests, but it doesn't seem to be firing off properly

@joshblack
Copy link
Contributor

@dakahn I think it'll need to be global instead of window, potentially, since it's saying ResizeObserver is undefined and so it's not resolving it from the global scope

@dakahn
Copy link
Contributor Author

dakahn commented Sep 9, 2021

Hmmm -- @joshblack something funnier is going on I don't understand. If I lift up window.ResizeOBserver = .... globally it works (like I would expect), but if I wrap it in beforeEach it breaks, but maybe beforeEach has to be inside a describe block?

@joshblack
Copy link
Contributor

@dakahn yeah, I'd make sure that it's inside of the describe block for sure 👍 (let me know if I'm misunderstanding!)

@joshblack
Copy link
Contributor

@dakahn just pushed up a fix that should cover it

Copy link
Member

@sstrubberg sstrubberg left a comment

Choose a reason for hiding this comment

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

Still getting CORS errors on deploy preview.

@sstrubberg
Copy link
Member

Looks like the CORS errors are only present on Netlify's deploy preview. They work perfectly locally and in prod.

@sstrubberg sstrubberg enabled auto-merge (squash) September 24, 2021 14:20
@sstrubberg sstrubberg merged commit d6884e0 into carbon-design-system:main Sep 24, 2021
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.

Issue with using ExpandableTile within a Tab
4 participants