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

advance analytics plotly chart components #1266

Merged

Conversation

muhammad-ammar
Copy link
Contributor

@muhammad-ammar muhammad-ammar commented Jul 24, 2024

JIRA:

Screenshot:
Screenshot 2024-07-29 at 16-39-19 AnalyticsV2 - edX Admin Portal


For all changes

  • Ensure adequate tests are in place (or reviewed existing tests cover changes)

Only if submitting a visual change

  • Ensure to attach screenshots
  • Ensure to have UX team confirm screenshots

@muhammad-ammar muhammad-ammar force-pushed the ammar/advance-analytics-plotly-chart-components branch from ff6026b to d708471 Compare July 24, 2024 09:39
@muhammad-ammar muhammad-ammar changed the title Ammar/advance analytics plotly chart components advance analytics plotly chart components Jul 24, 2024
Copy link

codecov bot commented Jul 24, 2024

Codecov Report

Attention: Patch coverage is 87.77778% with 11 lines in your changes missing coverage. Please review.

Project coverage is 85.43%. Comparing base (0639733) to head (cabb37c).
Report is 67 commits behind head on master.

Files with missing lines Patch % Lines
.../components/AdvanceAnalyticsV2/AnalyticsV2Page.jsx 16.66% 10 Missing ⚠️
...c/components/EnterpriseApp/EnterpriseAppRoutes.jsx 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1266      +/-   ##
==========================================
+ Coverage   85.41%   85.43%   +0.01%     
==========================================
  Files         542      554      +12     
  Lines       11991    12081      +90     
  Branches     2525     2567      +42     
==========================================
+ Hits        10242    10321      +79     
- Misses       1691     1701      +10     
- Partials       58       59       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@muhammad-ammar muhammad-ammar marked this pull request as draft July 24, 2024 15:42
@muhammad-ammar muhammad-ammar force-pushed the ammar/advance-analytics-plotly-chart-components branch 2 times, most recently from 4adbdb5 to 275e827 Compare July 25, 2024 12:28
@muhammad-ammar muhammad-ammar marked this pull request as ready for review July 26, 2024 04:28
Copy link
Contributor

@saleem-latif saleem-latif 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, just a couple of comments.

Also, can you mark all the strings for i18n as well?


const AnalyticsV2Page = () => {
const [activeTab, setActiveTab] = useState('enrollments');
const [granularity, setGranularity] = useState(undefined);
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be better to have the default value here instead of undefined.

const AnalyticsV2Page = () => {
const [activeTab, setActiveTab] = useState('enrollments');
const [granularity, setGranularity] = useState(undefined);
const [calculation, setCalculation] = useState(undefined);
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.

@saleem-latif
Copy link
Contributor

@muhammad-ammar Please attach screenshots in the PR description.

Copy link
Contributor

@mahamakifdar19 mahamakifdar19 left a comment

Choose a reason for hiding this comment

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

The strings need to be marked for i18n. Other than that, LGTM

Copy link
Contributor

@jajjibhai008 jajjibhai008 left a comment

Choose a reason for hiding this comment

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

  1. [Request] Please mark all static strings throughout your changes for translations.
  2. [Question] Aren't we supposed to apply loaders on each graph level? Or we supposed to apply a data leader on each tab level? Is that work part of this ticket?

<div className="container-fluid w-100">
<div className="row data-refresh-msg-container mb-4">
<div className="col">
<span>Data updated on ...</span>
Copy link
Contributor

Choose a reason for hiding this comment

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

Please mark the static strings for translations.

<div className="row filter-container mb-4">
<div className="col">
<Form.Group>
<Form.Label>Start Date</Form.Label>
Copy link
Contributor

Choose a reason for hiding this comment

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

Please mark the static strings for translations.

@muhammad-ammar
Copy link
Contributor Author

muhammad-ammar commented Jul 29, 2024

  1. [Question] Aren't we supposed to apply loaders on each graph level? Or we supposed to apply a data leader on each tab level? Is that work part of this ticket?
  • We will add loaders once we integrate APIs. I am not sure but I think adding loaders for charts may not be straight forward because plotly takes some time to plot the chart.

@muhammad-ammar muhammad-ammar force-pushed the ammar/advance-analytics-plotly-chart-components branch from cc5c2f9 to cabb37c Compare July 31, 2024 03:58
@muhammad-ammar muhammad-ammar merged commit 2dad691 into master Jul 31, 2024
6 checks passed
@muhammad-ammar muhammad-ammar deleted the ammar/advance-analytics-plotly-chart-components branch July 31, 2024 11:03
@muhammad-ammar muhammad-ammar added the plotly-migration Work to migrate from Plotly DASH to PlotlyJS label Sep 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plotly-migration Work to migrate from Plotly DASH to PlotlyJS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants