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

Add NoDataAlert widget #188

Conversation

Clebal
Copy link
Contributor

@Clebal Clebal commented Oct 1, 2021

@Clebal Clebal requested a review from VictorVelarde October 1, 2021 08:53
@shortcut-integration
Copy link

@coveralls
Copy link
Collaborator

coveralls commented Oct 1, 2021

Pull Request Test Coverage Report for Build 1295492895

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.1%) to 74.407%

Totals Coverage Status
Change from base Build 1291465225: -0.1%
Covered Lines: 695
Relevant Lines: 866

💛 - Coveralls

@VictorVelarde VictorVelarde changed the title Add component for showing an alert in widgets when there is no data available Add NoDataAlert widget Oct 1, 2021
@VictorVelarde
Copy link
Contributor

I've added it also to FormulaWidget, for consistency (and because default 0.00 value for empty might be wrong / misleading under certain operations). I also reduced a bit the IMO very long text (@borja-munoz can maybe give us feedback on that during QA)

image

Copy link
Contributor

@VictorVelarde VictorVelarde left a comment

Choose a reason for hiding this comment

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

LGTM, so 🚀

@Clebal
Copy link
Contributor Author

Clebal commented Oct 1, 2021

Sorry @VictorVelarde, I don't agree. From my experience in PS projects, when the count of elements is 0, the client wants to see 0, no "No data". That's why I didn't add it there.

Another example, if the sum of revenue of a certain store is 0, it isn't the same as no data.

Maybe I'd add it when formulaData is null or undefined.

@VictorVelarde
Copy link
Contributor

VictorVelarde commented Oct 1, 2021

I understand that displaying 0.0 might be fine assuming Count or Sum, but there are other operations that can be applied to Formula, where 0 is not, IMO, correct. Having said that I have no issues in reverting that commit, let's just ping @borja-munoz for confirmation on this before merging it

@VictorVelarde
Copy link
Contributor

VictorVelarde commented Oct 1, 2021

I'm gonna do one thing, I'm gonna revert my commit 0d021bf, so to merge this as Sergio set it, and if Borja wants to include my FormulaWidget change, we can do another PR with cherry-pick later

@VictorVelarde VictorVelarde force-pushed the feature/sc-183641/add-component-for-showing-an-alert-in-widgets branch from 0d021bf to c47f49a Compare October 1, 2021 14:44
@VictorVelarde VictorVelarde merged commit af244af into master Oct 1, 2021
@VictorVelarde VictorVelarde deleted the feature/sc-183641/add-component-for-showing-an-alert-in-widgets branch October 1, 2021 14:45
@VictorVelarde VictorVelarde mentioned this pull request Oct 1, 2021
8 tasks
@borja-munoz
Copy link
Contributor

I think for COUNT operations makes sense to show 0 if there is no data in the current viewport. For the other aggregation operations (SUM, AVG, MIN, MAX), I think it is better to show the no data message.

Sorry for introducing a third option. If there is no time now to implement it, I'd keep Sergio's approach for now, but revisit this in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants