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

[IRON-14173] - Make updates to force scroll #71

Merged
merged 4 commits into from
Feb 16, 2023

Conversation

kelvinjue
Copy link
Contributor

@kelvinjue kelvinjue commented Feb 6, 2023

IRON-14173

Bug fix for force scroll. Issue would arise when the prop wasn't passed in for a group, but it was set on the group settings. The prop would result in passing back something like force_scroll: undefined, which would conflict with the group setting. The changes include clearing out fields that are undefined from the group options.

Also making an update to fix behavior attached to a change for retrieveHTML. This would alter the behavior of retrieveHTML, so that it wouldn't reset virtual group properties. But the React SDK depends on that behavior right now to pull settings from the site.

@kelvinjue kelvinjue requested a review from a team February 6, 2023 17:37
@kelvinjue kelvinjue self-assigned this Feb 6, 2023
@kelvinjue kelvinjue changed the title [IRON-14173] - Make updates to force scroll + retrieveHTML [IRON-14173] - Make updates to force scroll Feb 6, 2023
@Wilsonl9
Copy link
Contributor

@kelvinjue Hmm, thats pretty odd. Normally, doesnt undefined get left out of the objects? in this case, they are actually overwriting the values with undefined?

@kelvinjue
Copy link
Contributor Author

@kelvinjue Hmm, thats pretty odd. Normally, doesnt undefined get left out of the objects? in this case, they are actually overwriting the values with undefined?

Yup, that's the bug. You'll see in the Group object that some attributes are defined as { force_scroll: undefined }. Having a value of a field be undefined won't drop the entire key/value entry out.

@Wilsonl9
Copy link
Contributor

@kelvinjue Hmm, thats pretty odd. Normally, doesnt undefined get left out of the objects? in this case, they are actually overwriting the values with undefined?

Yup, that's the bug. You'll see in the Group object that some attributes are defined as { force_scroll: undefined }. Having a value of a field be undefined won't drop the entire key/value entry out.

ahh wait, I think it got it mixed up with mongo where undefined wouldnt be saved? I think this makes sense 👍

@Wilsonl9
Copy link
Contributor

Something else I am curious about, by doing this, users would not be able to overwrite and "clear" their group settings. Is that something we should be worried about?

@kelvinjue
Copy link
Contributor Author

Something else I am curious about, by doing this, users would not be able to overwrite and "clear" their group settings. Is that something we should be worried about?

I'm not sure I understand the case here. If a prop is passed in as undefined or not passed in at all, then we wouldn't send anything back to the group, and it'd fallback onto the defaults configured in our setup assistant. This bug happens because of properties being overwritten and putting the group in a bad state.

Copy link
Contributor

@Wilsonl9 Wilsonl9 left a comment

Choose a reason for hiding this comment

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

LGTM, nice find!

@kelvinjue kelvinjue merged commit 59d44a9 into main Feb 16, 2023
@mewelling mewelling deleted the kelvinjue/IRON-14173/fix-force-scroll branch October 12, 2023 14:45
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