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

[Hotfix] Cohort dashoard charts and Visibility #1818

Merged
merged 3 commits into from
Jan 22, 2024

Conversation

Codebmk
Copy link
Member

@Codebmk Codebmk commented Jan 22, 2024

Summary of Changes (What does this PR do?)

  • Modified averages and exceedances x-axis labels to show devices for cohorts view
  • Modified appearance of x-axis labels for cohorts charts
  • Set the default visibility value to null if a device has no visibility value.

Status of maturity (all need to be checked before merging):

  • I've tested this locally
  • I consider this code done
  • This change ready to hit production in its current state

Screenshots (optional)

image

Copy link
Contributor

New netmanager changes available for preview here

@Codebmk Codebmk marked this pull request as draft January 22, 2024 08:02
@Codebmk Codebmk marked this pull request as ready for review January 22, 2024 08:11
Copy link
Contributor

New netmanager changes available for preview here

@@ -186,7 +186,7 @@ const CohortForm = ({ cohort }) => {
const initialState = {
name: '',
network: activeNetwork.net_name,
visibility: false
visibility: null
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi @Codebmk , thanks for this.

  • Is null the most appropriate default value for a Boolean field?
  • Could you share some more insights behind this thought process of this hotfix?

Copy link
Member Author

Choose a reason for hiding this comment

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

The default was initially false but that is not the case with the backend. If there's no visibility value, it is empty. Hence using null in the frontend. @Baalmart

Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay @Codebmk . thanks for the clarifications. As long as things work as expected. Let us continue to study the situation.

@Baalmart Baalmart merged commit 2ccd405 into staging Jan 22, 2024
27 checks passed
@Baalmart Baalmart deleted the hotfix-cohort-visibility branch January 22, 2024 19:41
@Baalmart Baalmart mentioned this pull request Jan 22, 2024
2 tasks
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.

2 participants