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

Call getSize in getCorrectedSize #2520

Merged

Conversation

rachel-fenichel
Copy link
Collaborator

The basics

  • I branched from render/collab
  • My pull request is against render/collab
  • My code follows the style guide

The details

Resolves

Changing the field value didn't update the appearance of the field, even though the value was changed internally.

Proposed Changes

Call getSize from getCorrectedSize

Reason for Changes

getSize rerenders the field if necessary. I don't want to duplicate its internal logic, especially since that's an active area of code, so I'm just calling it here.

Another option would be to call getSize and then getCorrectedSize in sequence in measurables.js. That would put it in only one place, which would be nice, but it's also a weird place for it.

@RoboErikG
Copy link
Contributor

What's the plan for getSize/getCorrectedSize once the new renderer is the default? What does the eventual cleanup look like?

@rachel-fenichel
Copy link
Collaborator Author

Eventual cleanup plan:
For every field that I needed to write getCorrectedSize, there's something wonky in the current rendering that leads to a weird offset. I will go through one field at a time and simultaneously remove the offset and the call to getCorrectedSize.

Copy link
Contributor

@RoboErikG RoboErikG left a comment

Choose a reason for hiding this comment

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

SGTM

@rachel-fenichel rachel-fenichel merged commit db15633 into google:render/collab May 29, 2019
@rachel-fenichel rachel-fenichel deleted the render/update_fields branch May 29, 2019 19:42
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.

None yet

2 participants