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

(analytics): Frontend updates for Application dashboard and data source CRUD v2 #2890

Merged
merged 9 commits into from
Jun 14, 2021

Conversation

ishangupta-ds
Copy link
Member

@ishangupta-ds ishangupta-ds commented Jun 11, 2021

Proposed changes

  • Dashboard options:
    • Clone
    • Download
    • Configure
  • Bug and style fixes on application dashboard
  • Data source create v2
  • Data source update v2
  • Create dashboard v2
    • Pre defined
    • Custom
    • Upload
    • Integration with series and label values API for autocompleting / suggesting metric queries
    • Resource discovery integration for automated query generation based on prometheus labels (to be also used for selection on app. dashboard)
  • Update dashboard v2
    • Edit metadata
    • Edit panels
    • Edit queries
    • Custom Prometheus mode using Ace editor with prometheus highlight rules, theme, prometheus query functions and series / label / value completions.
  • Added portal-dashboards directory in the monitoring folder

Types of changes

  • New feature (non-breaking change which adds functionality)
  • Bugfix (non-breaking change which fixes an issue)

Checklist

  • I have read the CONTRIBUTING doc
  • I have signed the commit for DCO to be passed.
  • Lint and unit tests pass locally with my changes

Dependency

Special notes for your reviewer:

App. dashboard (with data source) CRUD v2 PR #2

Signed-off-by: ishangupta-ds <[email protected]>
Signed-off-by: ishangupta-ds <[email protected]>
Comment on lines 1 to 9
export const copyTextToClipboard = (text: string) => {
if (!navigator.clipboard) {
console.error('Oops Could not copy text: ');
return;
}
navigator.clipboard
.writeText(text)
.catch((err) => console.error('Async: Could not copy text: ', err));
};
Copy link
Member

Choose a reason for hiding this comment

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

While this is a good change, we have been using this function to pass it the local state from a react component and base the copying state position from that, This was discussed earlier as well..I did not see this being used in this PR, have you tested its functionality?

Copy link
Member Author

Choose a reason for hiding this comment

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

Its being used to copy the queries @vanshBhatia-A4k9

Copy link
Member Author

Choose a reason for hiding this comment

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

Had tested it, will check again though ! Confirming shortly.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

@vanshBhatia-A4k9 vanshBhatia-A4k9 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 the work. Please address my doubt when possible.

@vanshBhatia-A4k9
Copy link
Member

Also, Since the overview tab has changed entirely now, please update it here as well

Copy link
Member

@imrajdas imrajdas left a comment

Choose a reason for hiding this comment

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

Please fix the package-lock.json

Signed-off-by: ishangupta-ds <[email protected]>
Signed-off-by: ishangupta-ds <[email protected]>
@ishangupta-ds
Copy link
Member Author

Please fix the package-lock.json

@rajdas98 on running the npm install command i am getting these changes, should i just revert to master and send without running it?

@ishangupta-ds
Copy link
Member Author

Please fix the package-lock.json

Fixed using lts PTAL @rajdas98

import { ParsedPrometheusData } from '../models/dashboardsData';
import { PromQuery } from '../models/graphql/dashboardsDetails';
/* eslint-disable no-useless-escape */
/* eslint-disable no-param-reassign */
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you remove the eslint disable commands if possible?

Copy link
Member Author

@ishangupta-ds ishangupta-ds Jun 14, 2021

Choose a reason for hiding this comment

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

no-useless-escape we need for the regex and no-param-reassign is done purposely as per the logic (its just a warning not an error for lint)

@@ -0,0 +1,237 @@
/* eslint-disable no-unused-expressions */
Copy link
Contributor

Choose a reason for hiding this comment

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

same

Copy link
Member Author

@ishangupta-ds ishangupta-ds Jun 14, 2021

Choose a reason for hiding this comment

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

Will try to but, can't remove this no-param-reassign as lint wants to return something on iterating but its not my use-case, its a kind of warning but we have purposely used the map/forEach this way. @Saranya-jena

@ishangupta-ds
Copy link
Member Author

@Saranya-jena i have removed as much disabled lint checks as possible but some are purposely placed.

@ishangupta-ds ishangupta-ds merged commit 3ef78a1 into litmuschaos:master Jun 14, 2021
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.

4 participants