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.

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

feat: implement scores distributions charts #157

Merged
merged 10 commits into from
Jun 19, 2024

Conversation

Marc-AntoineA
Copy link
Collaborator

What

Create small vega histograms for current results nova, eco and nutri-score distributions.

Screenshot

image

Fixes bug(s)

#123

Part of

@Marc-AntoineA Marc-AntoineA self-assigned this Jun 11, 2024
@Marc-AntoineA Marc-AntoineA marked this pull request as draft June 11, 2024 18:41
Copy link
Member

@alexgarel alexgarel left a comment

Choose a reason for hiding this comment

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

Thanks for this work @Marc-AntoineA

I think we should talk about it in a small meeting to decide next steps.

frontend/src/search-charts.ts Outdated Show resolved Hide resolved
frontend/public/off.html Outdated Show resolved Hide resolved
frontend/src/search-charts.ts Outdated Show resolved Hide resolved
frontend/src/search-charts.ts Outdated Show resolved Hide resolved
frontend/src/search-charts.ts Outdated Show resolved Hide resolved
frontend/src/search-charts.ts Outdated Show resolved Hide resolved
frontend/src/search-charts.ts Outdated Show resolved Hide resolved
frontend/src/search-charts.ts Outdated Show resolved Hide resolved
frontend/src/search-charts.ts Outdated Show resolved Hide resolved
frontend/src/search-charts.ts Outdated Show resolved Hide resolved
@teolemon teolemon added charts ✨ enhancement New feature or request labels Jun 12, 2024
* fix: override updated function (and create a vegaRepresentation object)
* feat: raise error if vega does not exist ;
* lint: add typescript types and deactivate "no-ts-ignore" rule
@Marc-AntoineA Marc-AntoineA linked an issue Jun 19, 2024 that may be closed by this pull request
@Marc-AntoineA Marc-AntoineA marked this pull request as ready for review June 19, 2024 10:08
@Marc-AntoineA Marc-AntoineA requested a review from alexgarel June 19, 2024 10:08
Copy link
Member

@alexgarel alexgarel left a comment

Choose a reason for hiding this comment

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

@Marc-AntoineA it's ready to merge for me.

I added two comment, it's up to you to decide if you do it in a new PR or in this PR, but try not to loose the information :-)

Comment on lines 33 to 44
try {
vega;
} catch (e) {
if (e instanceof ReferenceError) {
console.error(
'Vega is required to use searchalicious-chart, you can import it using \
<script src="https://cdn.jsdelivr.net/npm/vega@5"></script>'
);
return html`<p>Please install vega to use searchalicious-chart</p>`;
}
throw e;
}
Copy link
Member

Choose a reason for hiding this comment

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

Instead of testing it every time, I would do this in firstUpdated:

    // eslint-disable-next-line @typescript-eslint/no-explicit-any
    override firstUpdated(changedProperties: Map<any, any>) {
      super.firstUpdated(changedProperties);
      this.testVegaExists();
    }

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I set a boolean vegaInstalled in the constructor

frontend/src/search-chart.ts Outdated Show resolved Hide resolved
* do not check vega everytime
* use slot to allow "no-data" message
@Marc-AntoineA Marc-AntoineA merged commit 837a658 into main Jun 19, 2024
6 checks passed
@Marc-AntoineA Marc-AntoineA deleted the feat-implement-charts-123 branch June 19, 2024 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
charts ✨ enhancement New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

API / component proposal for Charts
3 participants