-
Notifications
You must be signed in to change notification settings - Fork 675
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
chore(litmus-portal): Home Page Stats for Litmus Portal. #2053
Conversation
Signed-off-by: ishangupta-ds <ishan.gupta@mayadata.io>
Signed-off-by: ishangupta-ds <ishan.gupta@mayadata.io>
Signed-off-by: ishangupta-ds <ishan.gupta@mayadata.io>
Signed-off-by: ishangupta-ds <ishan.gupta@mayadata.io>
Signed-off-by: ishangupta-ds <ishan.gupta@mayadata.io>
DefaultDBServerURL string = os.Getenv("DB_SERVER") | ||
DefaultAPISecret string = "litmus-portal@123" | ||
DefaultUserName string = "admin" | ||
DefaultUserPassword string = "litmus" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you change these back
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
already reverted. @arkajyotiMukherjee
const width: number = 2; | ||
const resultValue = ((value as number) / (maxValue as number)) * 100; | ||
useEffect(() => { | ||
return setColor('#5B44BA'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why does color require to be a state variable if I never changes.
use a fixed color from theme data with useTheme()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some components were already there just reused code from them, will make changes for this.. @arkajyotiMukherjee
const [color, setColor] = useState(' '); | ||
const width: number = 2; | ||
const resultValue = ((value as number) / (maxValue as number)) * 100; | ||
useEffect(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This useEffect is not needed
alt="Failed Icon" | ||
<Avatar | ||
style={{ | ||
backgroundColor: '#CA2C2C', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
put all colors in theme...if it is in react component use "useTheme" if it is used in styles...use normal theme value you get
/preview 20 LINK EXPIRES IN : 20mins
LINK EXPIRED |
/run-unit Test Status: The testing has been started please wait for the results ... |
/run-unit Test Status: The testing has been started please wait for the results ... |
Signed-off-by: ishangupta-ds <ishan.gupta@mayadata.io>
Changes made PTAL.
/run-unit Test Status: The testing has been started please wait for the results ... |
Signed-off-by: ishangupta-ds <ishan.gupta@mayadata.io>
Signed-off-by: ishangupta-ds <ishan.gupta@mayadata.io>
/run-unit Test Status: The testing has been started please wait for the results ... |
Signed-off-by: ishangupta-ds <ishan.gupta@mayadata.io>
/run-unit Test Status: The testing has been started please wait for the results ... |
Signed-off-by: ishangupta-ds <ishan.gupta@mayadata.io>
@rajdas98 @arkajyotiMukherjee @amityt @S-ayanide all the suggested changes have been made PTAL. |
/preview 10 LINK EXPIRES IN : 10mins
LINK EXPIRED |
litmus-portal/frontend/package.json
Outdated
@@ -17,6 +17,7 @@ | |||
"@material-ui/core": "^4.10.2", | |||
"@material-ui/icons": "^4.9.1", | |||
"@material-ui/pickers": "^3.2.10", | |||
"@types/lodash": "^4.14.161", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this should be a dev dependency
/* | ||
Reusable Semi Circular Progress Bar | ||
Required Params: value | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this comment, doesn't make sense in this context
This PR looks good to me, just a very minor change. 👍 |
WORKFLOW_DETAILS, | ||
{ | ||
variables: { projectID: userData.selectedProjectID }, | ||
fetchPolicy: 'cache-and-network', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove fetch policy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed @arkajyotiMukherjee
it('Pass Icon is present', () => { | ||
mount(wrapper); | ||
cy.get('[data-cy=passIcon]') | ||
.should('have.attr', 'src') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did we plan on removing all test cases @Jonsy13 @S-ayanide
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah we are not using these icons i am using the available icons in material ui as they are same, and moreover pngs were used earlier which were getting blurred... so these conditions will always fail @arkajyotiMukherjee cc: @Jonsy13 @S-ayanide
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No @arkajyotiMukherjee It's not decided.Tests will remain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
without icons how do i make the old test cases pass? @Jonsy13 @arkajyotiMukherjee
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ishangupta-ds Can we add the two tests but instead use you Avatar icon and other icons from MUI itself? If that's possible for now!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes @S-ayanide That should be good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not accessible as png now we have to check for the material icon, i have another PR pending for the analytics, will be adding test cases after this release @S-ayanide @Jonsy13 ..if it is good?
/run-unit Test Status: The testing has been started please wait for the results ... |
Signed-off-by: ishangupta-ds <ishan.gupta@mayadata.io>
@arkajyotiMukherjee @S-ayanide @Jonsy13 @rajdas98 PTAL. |
…#2053) This commit removes two outdated test cases from the cypress test suite of PassedVsFailed component Signed-off-by: ishangupta-ds <ishan.gupta@mayadata.io>
Proposed changes
Adding basic analytics for home page of litmus portal.
fixes #1788
This PR also removes two outdated test cases from the cypress test suite of PassedVsFailed component.
Types of changes
What types of changes does your code introduce to Litmus? Put an
x
in the boxes that applyChecklist
Put an
x
in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.Dependency
Special notes for your reviewer:
Analytics PR #1