Skip to content

Conversation

@aditi-bhatia
Copy link
Contributor

@aditi-bhatia aditi-bhatia commented Mar 7, 2024

Fixes #20413

This PR fixes an issue where bars for all graphs would turn a gradient green after navigating to a graph with empty data. Only categories with no data should display the gradient green bars and all other categories with data should have blue bars.


To Test:

  1. While on the Stats Traffic tab, click a category with empty data. You can use the mobile.blog site for testing which currently has 0 comments in the month view.
  2. You should see a green gradient bar for any category with empty data.
  3. Navigate to views or any other category with data. The bars should change to blue and not remain gradient green.

Video of bug occurring after clicking likes or comments (empty data) and then navigating back to views or visitors:

Screen_Recording_20240305_141423_Jetpack.Beta.mp4

Regression Notes

  1. Potential unintended areas of impact

    • TODO
  2. What I did to test those areas of impact (or what existing automated tests I relied on)

    • TODO
  3. What automated tests I added (or what prevented me from doing so)

    • TODO

PR Submission Checklist:

  • I have completed the Regression Notes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

Testing Checklist (strike-out the not-applying and unnecessary ones):

  • WordPress.com sites and self-hosted Jetpack sites.
  • Portrait and landscape orientations.
  • Light and dark modes.
  • Fonts: Larger, smaller and bold text.
  • High contrast.
  • Talkback.
  • Languages with large words or with letters/accents not frequently used in English.
  • Right-to-left languages. (Even if translation isn’t complete, formatting should still respect the right-to-left layout)
  • Large and small screen sizes. (Tablet and smaller phones)
  • Multi-tasking: Split screen and Pop-up view. (Android 10 or higher)

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Mar 7, 2024

WordPress📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.
App NameWordPress WordPress
FlavorJalapeno
Build TypeDebug
Versionpr20436-02daa2b
Commit02daa2b
Direct Downloadwordpress-prototype-build-pr20436-02daa2b.apk
Note: Google Login is not supported on these builds.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Mar 7, 2024

Jetpack📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.
App NameJetpack Jetpack
FlavorJalapeno
Build TypeDebug
Versionpr20436-02daa2b
Commit02daa2b
Direct Downloadjetpack-prototype-build-pr20436-02daa2b.apk
Note: Google Login is not supported on these builds.

Comment on lines 180 to 189
dataSet.color = ContextCompat.getColor(context, R.color.blue_50)
dataSet.setGradientColor(
ContextCompat.getColor(
context,
R.color.blue_50
), ContextCompat.getColor(
context,
R.color.blue_50
)
)
Copy link
Member

@irfano irfano Mar 8, 2024

Choose a reason for hiding this comment

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

This appears to be a bug from the chart library. Although we're updating dataSet of the BarChart, BarCharts BarChartRenderer is still using gradient colors from the previous chart.
I see a minor side effect of setting gradient colors again here. It causes the chart to render gradient color as a single color, an unnecessary process from the performance perspective.
May I suggest a simpler alternative solution? By setting the shader to null, we can prevent BarChartRenderers from drawing gradient. Wdyt?

Suggested change
dataSet.color = ContextCompat.getColor(context, R.color.blue_50)
dataSet.setGradientColor(
ContextCompat.getColor(
context,
R.color.blue_50
), ContextCompat.getColor(
context,
R.color.blue_50
)
)
chart.renderer.paintRender.shader = null
dataSet.color = ContextCompat.getColor(context, R.color.blue_50)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @irfano thanks for the suggestion! Yes, I agree that it's an issue with the library itself. I was concerned about the unnecessary process as well but noticed that BarChartViewHolder uses a similar workaround (lines 233-241 and 256-264):

            ContextCompat.getColor(
                context,
                R.color.stats_bar_chart_top
            ), ContextCompat.getColor(
                context,
                R.color.stats_bar_chart_top
            )
        )

I can go ahead and update the usage in TrafficBarChartViewHolder, but do you think it's a good idea to update these other occurrences as well?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I think we can update it on both places. I tested my suggestion and didn't notice any issues.

@aditi-bhatia
Copy link
Contributor Author

Hi @irfano, I pushed the changes we discussed above so it's ready if you'd like to take another look. Thanks!

@aditi-bhatia aditi-bhatia requested a review from irfano March 9, 2024 02:39
@sonarqubecloud
Copy link

sonarqubecloud bot commented Mar 9, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Copy link
Member

@irfano irfano left a comment

Choose a reason for hiding this comment

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

LGTM! 👍🏻

@irfano irfano merged commit 2bf30f7 into trunk Mar 9, 2024
@irfano irfano deleted the origin/issue/20413-gradient-bar-bug-fix branch March 9, 2024 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

The traffic chart bars are turning green

4 participants