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

feature/investor-preferences-screener #66

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

feature/investor-preferences-screener #66

wants to merge 21 commits into from
+1,507 −3

Conversation

jedrzejruta
Copy link

Created Investor Preferences Screener connector.

@jedrzejruta jedrzejruta added the enhancement New feature or request label Jan 13, 2025
@jedrzejruta jedrzejruta self-assigned this Jan 13, 2025
pawelfus
pawelfus requested changes Jan 13, 2025
View reviewed changes
Copy link
Contributor

@pawelfus pawelfus left a comment •
edited by jedrzejruta
Loading

Choose a reason for hiding this comment

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

Thanks!

In addition to the comments below:

demos/dashboards-investor-preferences/demo.html Outdated Show resolved Hide resolved
demos/dashboards-investor-preferences/demo.html Outdated Show resolved Hide resolved
demos/dashboards-investor-preferences/demo.html Show resolved Hide resolved
demos/dashboards-investor-preferences/demo.js Outdated Show resolved Hide resolved
@jedrzejruta jedrzejruta requested a review from pawelfus January 14, 2025 15:15
@jedrzejruta jedrzejruta requested a review from pawelfus January 17, 2025 17:11
pawelfus
pawelfus requested changes Jan 20, 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! See my comments inline

demos/dashboards-investor-preferences/demo.js Outdated Show resolved Hide resolved
demos/dashboards-investor-preferences/demo.js Outdated Show resolved Hide resolved
demos/dashboards-investor-preferences/demo.js Outdated Show resolved Hide resolved
DJTechnoo
DJTechnoo requested changes Jan 20, 2025
View reviewed changes
Copy link
Member

@DJTechnoo DJTechnoo left a comment

Choose a reason for hiding this comment

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

Looks amazing to me! Just small things:

docs/connectors/morningstar/screeners/investor-preferences.md Outdated Show resolved Hide resolved
demos/dashboards-investor-preferences/demo.js Outdated Show resolved Hide resolved
kamil-musialowski
kamil-musialowski requested changes Jan 23, 2025
View reviewed changes
Copy link
Contributor

@kamil-musialowski kamil-musialowski left a comment

Choose a reason for hiding this comment

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

Great work, just minor polishing and one side comment to discuss:

Do we want to investigate why that error happens, or it's just something weird on my side?
image

src/Screeners/InvestorPreferences/InvestorPreferencesConverter.ts Outdated Show resolved Hide resolved
src/Screeners/InvestorPreferences/InvestorPreferencesJSON.ts Show resolved Hide resolved
tests/Screeners/InvestorPreferences.test.ts Outdated Show resolved Hide resolved
pawelfus
pawelfus requested changes Jan 23, 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.

Some changes are required, overall looks good 👍

demos/dashboards-investor-preferences/demo.js Outdated Show resolved Hide resolved
demos/dashboards-investor-preferences/demo.js Show resolved Hide resolved
demos/dashboards-investor-preferences/demo.js Show resolved Hide resolved
demos/dashboards-investor-preferences/demo.js
cells: {
formatter () {
if (this.column.id === 'InvestorPreferences_StarRatingM255') {
return Array(this.value).fill(starIcon).join('');
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we set a minimum width for this column, o 5 stars fits nicely

Copy link
Author

@jedrzejruta jedrzejruta Jan 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Unfortunatelly, for now only two options for columns distribution are possible in DataGrid: full or fixed, and it's not possible to set a minimum width for particular column: https://api.highcharts.com/dashboards/#interfaces/DataGrid_Options.ColumnsSettings#distribution

@jedrzejruta
Copy link
Author

@kamil-musialowski

Do we want to investigate why that error happens, or it's just something weird on my side?

I get similar errors only when the code isn't built, after building the solution errors are no longer visible.

kamil-musialowski
kamil-musialowski approved these changes Jan 28, 2025
View reviewed changes
Copy link
Contributor

@kamil-musialowski kamil-musialowski left a comment

Choose a reason for hiding this comment

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

LGTM!

pawelfus
pawelfus requested 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.

Well done!

One extra thing, about docs. We need to add this to the list of available screeners. The list is in the other PR. Please merge in that PR here, and add link to this docs here: https://github.com/highcharts/connectors-morningstar/pull/67/files#diff-1e83393a0bf24a902c487a1f755be0c7aac05e7cf317c1dd2a2e2d9bf7887434

DJTechnoo
DJTechnoo approved these changes Jan 29, 2025
View reviewed changes
Copy link
Member

@DJTechnoo DJTechnoo left a comment

Choose a reason for hiding this comment

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

Amazing job!

@jedrzejruta jedrzejruta requested a review from pawelfus January 29, 2025 15:39
pawelfus
pawelfus approved these changes Jan 30, 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!

@pawelfus
Copy link
Contributor

Waiting for the final changes in #67

@pawelfus pawelfus self-requested a review January 31, 2025 10:19
@pawelfus pawelfus added this to the Next milestone Jan 31, 2025
pawelfus
pawelfus requested changes Jan 31, 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.

Please resolve the conflicts

@jedrzejruta jedrzejruta requested a review from pawelfus January 31, 2025 11:16
pawelfus
pawelfus approved these changes Jan 31, 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

@kamil-musialowski kamil-musialowski kamil-musialowski approved these changes

@bm64 bm64 bm64 approved these changes

@DJTechnoo DJTechnoo DJTechnoo approved these changes

@pawelfus pawelfus pawelfus approved these changes

@bre1470 bre1470 Awaiting requested review from bre1470

@TorsteinHonsi TorsteinHonsi Awaiting requested review from TorsteinHonsi

Assignees

@jedrzejruta jedrzejruta

Labels
enhancement New feature or request
Projects
None yet
Milestone
v1.1.0
Development

Successfully merging this pull request may close these issues.
5 participants
@jedrzejruta @pawelfus @DJTechnoo @bm64 @kamil-musialowski