Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Make memory address padding configurable #89
Make memory address padding configurable #89
Changes from all commits
c062c77
8894be9
fe0472f
3106c76
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have some misgivings about the number of places we're passing around the new memoized padding / length values when they're relevant to only one column, and we should avoid knowing too much about column internals outside of those contributions.
One idea about how to handle it would be to introduce a new lifecycle on the columns so that they can update any relevant state
onComponentDidUpdate
or whatever seems best. Another (obviously beyond the scope of this PR) would be to use something other than React so that individual columns could have tighter control of their own rendering.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean -- conceptually --
onComponentDidUpdate
inApp
, we invoke an update on each column, passing them a new object state to avoid having to pass column-specific state down to theMemoryWidget
/MemoryTable
?That sounds like a good idea to me!
I feel though such a change probably deserves a separate PR, wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, exactly. That way columns are kept up-to-date on changes in props/state and can store the results of any calculations they'd otherwise repeat.
I'd be inclined to do it here because this PR is introducing a new prop that's being passed in a style we might like to avoid, but if you feel strongly, we could put it off.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be frank: I agree with @planger that this should be addressed in a separate refactoring PR.
It is a great idea to have more fine granular control over updates. In the sense of avoiding to flood components with "unsed" or just passed through props. But it sounds like this is something that should be properly analyzed and rolled out across the board. I am sure there is more potential for this beyond the address padding. And a separate PR will give clearer separation of the feature enhancement and refactoring work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm ok with that approach, though I think it's not generally a good idea to acknowledge that something is not the direction we want to go and then move forward with it anyway. I've approved the PR - @planger, feel free to make whatever changes to the wording of the preference enum descriptions that you prefer and merge at your convenience.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @colin-grant-work ! I appreciate that the situation isn't ideal. Let's make sure that we address this as soon as we can.