Skip to content

Conversation

lerouxb
Copy link
Contributor

@lerouxb lerouxb commented Dec 7, 2023

To review: Maybe start with unifyDocuments() in unified-document.ts. Or alternatively just the ChangeView component in change-view.tsx. Don't be intimidated by the line count - it is almost all test fixtures' expected results.

Screenshot 2023-12-12 at 11 41 38

@lerouxb lerouxb added the feature flagged PRs labeled with this label will not be included in the release notes of the next release label Dec 7, 2023
return obj.changeType;
}

export function unifyDocuments(
Copy link
Contributor Author

@lerouxb lerouxb Dec 8, 2023

Choose a reason for hiding this comment

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

Maybe start here. GitHub collapsed this file..

color: palette.gray.dark2,
});

export function ChangeView({
Copy link
Contributor Author

@lerouxb lerouxb Dec 8, 2023

Choose a reason for hiding this comment

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

GitHub collapsed this file, but this is a good entry-point for the component if you prefer to read from that end.

@lerouxb lerouxb marked this pull request as ready for review December 8, 2023 16:47
? lookupValue((obj.right as Branch).path, right)
: undefined;

// TODO: BSONValue does not always show the bson type, so you can't spot bson type changes
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO is not really the right word, but I did want to call attention to this. Since we're using the same components as the Document component the BSON values behave the same. So a Double with the value of 1 or an Int32 with value 1 or a Long with the value 1 all look the same.

This might not be much of a problem. As long we keep the before/after values with their types they will be stringified as new Double(1), new Int32(1) and new Long(1) respectively before being diffed and therefore it will show up as changed. But the "red" and "green" values will then look identical.

I just bring this up because during tech design some people mentioned bson type changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(I removed the TODO)

@lerouxb lerouxb merged commit 7fc0b81 into main Dec 12, 2023
@lerouxb lerouxb deleted the add-change-view branch December 12, 2023 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feat feature flagged PRs labeled with this label will not be included in the release notes of the next release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants