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.

Sign up for GitHub

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

Jump to bottom

bugfix/xray-connector-invalid-response #56

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open

bugfix/xray-connector-invalid-response #56

wants to merge 18 commits into from
+531 −27

Conversation

kamil-musialowski
Copy link
Contributor

@kamil-musialowski kamil-musialowski commented Nov 27, 2024 •
edited
Loading

Fixed #54, response from xray converter was incorrect.

@kamil-musialowski kamil-musialowski self-assigned this Nov 27, 2024
@pawelfus pawelfus added this to the Next milestone Nov 28, 2024
@kamil-musialowski kamil-musialowski force-pushed the bugfix/xray-connector-invalid-response branch from 122730e to eb83551 Compare December 9, 2024 19:23
@kamil-musialowski
Copy link
Contributor Author

force push due to incorrect commit name, missing GlobalStockSector

@pawelfus pawelfus removed their request for review January 8, 2025 09:17
@kamil-musialowski kamil-musialowski marked this pull request as ready for review January 8, 2025 12:42
pawelfus
pawelfus requested changes Jan 8, 2025
View reviewed changes
Copy link
Contributor

@pawelfus pawelfus left a comment

Choose a reason for hiding this comment

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

Nice!

In addition to the inline comments, there is a bug when scrolling down in tables. Is this a bug in DataGrid (please report it then) or in the implementation (please fix it then)?

image

demos/dashboards-xray/demo.html Outdated Show resolved Hide resolved
demos/dashboards-xray/demo.js Outdated Show resolved Hide resolved
demos/dashboards-xray/demo.js Outdated Show resolved Hide resolved
demos/dashboards-xray/demo.js Outdated Show resolved Hide resolved
demos/dashboards-xray/demo.js Outdated Show resolved Hide resolved
pawelfus
pawelfus reviewed Jan 9, 2025
View reviewed changes
Copy link
Contributor

@pawelfus pawelfus left a comment

Choose a reason for hiding this comment

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

Thanks!

jedrzejruta
jedrzejruta approved these changes Jan 13, 2025
View reviewed changes
Copy link

@jedrzejruta jedrzejruta left a comment

Choose a reason for hiding this comment

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

Nicely done! 🎉

@pawelfus pawelfus requested a review from bre1470 January 14, 2025 11:05
bre1470
bre1470 reviewed Jan 15, 2025
View reviewed changes
src/XRay/XRayJSON.ts Outdated Show resolved Hide resolved
bre1470
bre1470 approved these changes Jan 15, 2025
View reviewed changes
Copy link
Member

@bre1470 bre1470 left a comment

Choose a reason for hiding this comment

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

Otherwise it looks good. Some unit tests would be nice though, to test against the new columns.

pawelfus
pawelfus approved these changes Jan 17, 2025
View reviewed changes
Copy link
Contributor

@pawelfus pawelfus left a comment

Choose a reason for hiding this comment

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

Thanks!

bre1470
bre1470 approved these changes Jan 17, 2025
View reviewed changes
src/XRay/XRayJSON.ts Outdated Show resolved Hide resolved
bre1470
bre1470 approved these changes Jan 23, 2025
View reviewed changes
Copy link
Member

@bre1470 bre1470 left a comment

Choose a reason for hiding this comment

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

Thank you! Looks good. 👍

@kamil-musialowski kamil-musialowski marked this pull request as draft January 23, 2025 15:56
@kamil-musialowski
Copy link
Contributor Author

Switching to draft, need to add missing returns.

jedrzejruta
jedrzejruta approved these changes Jan 27, 2025
View reviewed changes
Copy link

@jedrzejruta jedrzejruta left a comment

Choose a reason for hiding this comment

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

Looks good, great work! 🎉

@kamil-musialowski kamil-musialowski marked this pull request as ready for review January 28, 2025 06:56
pawelfus
pawelfus approved these changes Jan 29, 2025
View reviewed changes
Copy link
Contributor

@pawelfus pawelfus left a comment

Choose a reason for hiding this comment

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

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bm64 bm64 bm64 approved these changes

@jedrzejruta jedrzejruta jedrzejruta approved these changes

@bre1470 bre1470 bre1470 approved these changes

@DJTechnoo DJTechnoo DJTechnoo approved these changes

@pawelfus pawelfus pawelfus approved these changes

Assignees

@kamil-musialowski kamil-musialowski

Labels
None yet
Projects
None yet
Milestone
Next
Development

Successfully merging this pull request may close these issues.

XRayConnector doesn't return valid response for Portfolio data points
6 participants
@kamil-musialowski @pawelfus @DJTechnoo @bre1470 @jedrzejruta @bm64