-
Notifications
You must be signed in to change notification settings - Fork 167
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
Esefah meng history strip #535
base: esefah-meng
Are you sure you want to change the base?
Conversation
This pull request introduces 4 alerts when merging fdf2c63 into f805c6d - view on LGTM.com new alerts:
|
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.
nice work! i've left comments that i hope are helpful. since it's a working branch, i'll leave it up to you what level of detail you want to address them to before merging
I have pushed changes to address your comments. Please send any feedback especially for cleanHistorySpec in HistortyList.tsx. Not sure if that is the right/ok way to go about removing interactions from the history previews. |
This pull request introduces 11 alerts when merging 82b2fa4 into f805c6d - view on LGTM.com new alerts:
|
This pull request introduces 11 alerts when merging ba9b9fc into f805c6d - view on LGTM.com new alerts:
|
This pull request introduces 1 alert and fixes 1 when merging 8515e2e into f805c6d - view on LGTM.com new alerts:
fixed alerts:
|
const scaleInfo = getScaleInfoForGroup(state, group._id); | ||
const fieldsOfGroup = getFieldsOfGroup(state, group._id); | ||
const scaleInfo = getScaleInfoForGroup(storeState, group._id); | ||
const fieldsOfGroup = getFieldsOfGroup(storeState, group._id); |
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.
now that i think about it, these should use visState too. if we pass in an older visState to the exporter, we expect it to get the scale info for that state. using the store state will use the current scale info instead, which could cause problems.
this means that the getScaleInfoForGroup and getFieldsOfGroup functions should probably be edited to use visState as well, and all uses of those functions updated accordingly. could you add one more commit with that change?
great work implementing the earlier feedback! your approach in cleanHistorySpec looks right to me. left one comment for one last refactoring task then you're good to go |
This PR includes:
TODOs amongst bigger todos: