CSS and other bug fixes for 2.0 beta 1 #2535

ishangupta-ds · 2021-03-12T12:43:24Z

Proposed changes

CSS bugs fixed
Minor bug fixes for UI

Types of changes

What types of changes does your code introduce to Litmus? Put an x in the boxes that apply

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

Special notes for your reviewer:

Bug-fix PR

Signed-off-by: ishangupta-ds <[email protected]>

whitetiger1399 · 2021-03-12T12:58:26Z

litmus-portal/frontend/public/locales/en/translation.yaml

@@ -30,7 +30,7 @@ welcomeModal:

 sidebar:
  title: Litmus
-  version: 'Version: 1.13'
+  version: "Version: 1.13"



Please remove double/single quotes as it is not required. Only where we have to use : as part of the string we require to write it in quotes. Also kindly do remove such quotes which are not required at all places in the translation file

sure these changes took place automatically due to the editor configurations

will revert to the original file @whitetiger1399

Also, do the necessary changes at other places also. We can take it up as part of refactoring.

This PR cannot include refactoring changes due to the deadlines of beta 1. Patch releases can be done @whitetiger1399 cc: @rajdas98

the yaml file doesn't require strings in "".
Please follow consistent typing. This shouldn't go as a separate patch. Don't fix what's not broken :)

remove quotes from yaml string. not needed

Yes i have already mentioned that i am reverting to the original file @arkajyotiMukherjee PTAL

whitetiger1399 · 2021-03-12T13:01:20Z

litmus-portal/frontend/src/components/WorkflowCard/styles.ts

@@ -42,16 +42,16 @@ const useStyles = makeStyles((theme) => ({
    margin: theme.spacing(1),
    textAlign: 'center',
    cursor: 'pointer',
-    border: `1px solid ${theme.palette.text.hint}`,


Can we replace px with some other relative unit at this place and other places also

yes @whitetiger1399 , I have just fixed the colours as these components are being removed as part of the v2 redesign so increasing the number of changes with this, won't be of much use here. We may refactor the code while redesigning which is already in progress on a separate branch.

For border px is fine...you can change but it's fine...too small a thing... you won't notice it being responsive.

whitetiger1399 · 2021-03-12T13:04:08Z

litmus-portal/frontend/src/components/YamlEditor/Editor.tsx

@@ -118,7 +118,7 @@ const YamlEditor: React.FC<YamlEditorProps> = ({
        'ace_gutter-cell'
      );
      for (let i = 0; i < nodeStyleErrorList.length; i += 1) {
-        (nodeStyleErrorList[i] as any).style.backgroundColor = '#1C1C1C';


Kindly import colors from theme file if possible instead of hardcoded colors

These changes will be with v2 redesigning.

If we are making any change in the Hex then it can be from litmus theme itself now also as hard coded colors do not look good.

There are two different colours in the editor right now, latest code is available on v2 branch

This change makes the colour uniform as was in the design before some breaking changes.

use useTheme() to take the colors rom theme file

litmus-portal/frontend/src/views/Community/GeoMap/styles.ts

whitetiger1399 · 2021-03-12T13:06:59Z

litmus-portal/frontend/src/pages/HomePage/style.ts

@@ -67,6 +78,12 @@ const useStyles = makeStyles((theme) => ({
  },

  arrowForwardIcon: {
+    color: theme.palette.primary.main,
+    marginLeft: theme.spacing(1),


Kindly avoid giving margin in negative numbers. Try to rearrange the div or its children position.

Here its required @whitetiger1399 , it has been used at many places with negative margins.

The optimization may be done in patch releases or else again the changes would increase because of code refactoring.

Making this change won't increase the number of file change as it is already being changed. We can take it up as part of refactoring now and not delay it
cc: @arkajyotiMukherjee

Delaying it makes no sense... it'll be always put in a backlog... please make all required changes now...

Patch releases should be for unknown bugs after release...not for known problems in an unmerged PR

Path releases are for fixing unknown bugs...let's not use it to push know fixes to an unmerged PR.

ok for the above class i am updating, for all other places we might need another PR @arkajyotiMukherjee

It's fine but then you need to take responsibility and track all the files with filenames where the changes need to be done. @ishangupta-ds

@whitetiger1399 any reasons for avoiding negative margins? I don't see any harm in using it.

whitetiger1399 · 2021-03-12T13:13:08Z

litmus-portal/frontend/src/pages/HomePage/style.ts

@@ -105,19 +122,22 @@ const useStyles = makeStyles((theme) => ({
    flexDirection: 'row',
    marginTop: theme.spacing(3.75),
    marginBottom: theme.spacing(2),
-    [theme.breakpoints.down('md')]: {


Kindly use standard and common breakpoints rather than a specific number

This again is required as the standard ones did not work correctly. @whitetiger1399

The standard ones should work. You may try with other standard breakpoints and set the most suitable one rather than coming up with a specific number that is not as per Litmus UI

They are not working due to the design

litmus-portal/frontend/src/views/Community/CommunityTimeSeriesPlot/styles.ts

whitetiger1399 · 2021-03-12T13:18:59Z

litmus-portal/frontend/src/views/CreateWorkflow/VerifyCommit/styles.ts

@@ -149,5 +149,10 @@ const useStyles = makeStyles((theme: Theme) => ({
  buttomPad: {
    paddingBottom: theme.spacing(3.75),
  },
+  closeBtn: {
+    color: theme.palette.secondary.contrastText,


Kindly avoid negative values for margins and try rearranging the divs. cc : @arkajyotiMukherjee

@whitetiger1399 the litmus-ui modal is placing the button inside the content so this cannot be changed or the display is blocked by this button.

Have updated at other places but here it's not working without negative margins because of the way the modal was built.

It's better to raise the a PR to litmus-ui and avoid unwanted coding practices.
Can you track this change and make sure to remove it after updating litmus-ui @ishangupta-ds

whitetiger1399 · 2021-03-12T13:20:41Z

litmus-portal/frontend/src/views/CreateWorkflow/VerifyCommit/index.tsx

@@ -281,7 +281,9 @@ const VerifyCommit: React.FC<VerifyCommitProps> = ({
        onClose={handleClose}
        width="60%"
        modalActions={
-          <ButtonOutlined onClick={handleClose}>&#x2715;</ButtonOutlined>


Can we use icon from Litmus as per Figma rather than Unicode character being passed as children to button component? cc: @arkajyotiMukherjee

This is essentially a colour fix PR @whitetiger1399 cc: @arkajyotiMukherjee All modals are using such Unicode values, file changes will be 50+ if all changes are made in this PR itself.

I don't believe it'll cross that many files... unicode is fine if it looks exactly like the design

I have not updated any code just added a class file @whitetiger1399

ishangupta-ds · 2021-03-12T13:21:21Z

@vanshBhatia-A4k9 Please pull the entire code to test instead of adding changes to certain parts, you are missing some changes in other files which is why the graph isn't resizing PTAL at the file changes.

vanshBhatia-A4k9 · 2021-03-12T13:28:57Z

@vanshBhatia-A4k9 Please pull the entire code to test instead of adding changes to certain parts, you are missing some changes in other files which is why the graph isn't resizing PTAL at the file changes.

Sure @ishangupta-ds I will do so, but the point was that there are no responsive designs with us, and adding responsiveness without them wouldn't be the correct approach.

ishangupta-ds · 2021-03-12T13:34:58Z

@vanshBhatia-A4k9 Please pull the entire code to test instead of adding changes to certain parts, you are missing some changes in other files which is why the graph isn't resizing PTAL at the file changes.

Sure @ishangupta-ds I will do so, but the point was that there are no responsive designs with us, and adding responsiveness without them wouldn't be the correct approach.

Its not just a responsiveness fix, if you notice the current UI of the page even with a slightly reduced window size the graph and map leave the background paper which is not as per design. @vanshBhatia-A4k9

vanshBhatia-A4k9 · 2021-03-12T13:46:45Z

This is how it appears on my screen using the dev tools for the breakpoints

without devtools:

vanshBhatia-A4k9 · 2021-03-12T13:48:26Z

@vanshBhatia-A4k9 Please pull the entire code to test instead of adding changes to certain parts, you are missing some changes in other files which is why the graph isn't resizing PTAL at the file changes.

Sure @ishangupta-ds I will do so, but the point was that there are no responsive designs with us, and adding responsiveness without them wouldn't be the correct approach.

Its not just a responsiveness fix, if you notice the current UI of the page even with a slightly reduced window size the graph and map leave the background paper which is not as per design. @vanshBhatia-A4k9

Yes, you are correct, but as mentioned, reduced window sizes will mean making the designs more responsive, which it is presently not and we don''t have them in figma either AFAIK

PTAL at latest changes, all comments are on outdated files.

arkajyotiMukherjee

Please create an issue to track the requested changes which can not be accommodated now...and assign yourself the same

arkajyotiMukherjee · 2021-03-12T16:39:54Z

...rtal/frontend/src/views/AnalyticsDashboard/LitmusDashboard/WorkflowComparisonTable/styles.ts

@@ -382,4 +379,10 @@ export const StyledTableCell = withStyles((theme) =>
  })
 )(TableCell);

+export const StyledCheckbox = withStyles((theme) => ({


then please make it a component and import form the component folder

arkajyotiMukherjee · 2021-03-12T16:42:16Z

litmus-portal/frontend/src/views/Community/GeoMap/styles.ts

@@ -29,34 +29,35 @@ const useStyles = makeStyles((theme) => ({
    },
  },
  cityMapComposableMap: {
-    width: '54rem',
-    height: '40rem',
+    width: '45vw',


Make "Removing plotly and using litmus-ui graphs" an action item for the next release please.

arkajyotiMukherjee · 2021-03-12T16:45:12Z

litmus-portal/frontend/src/views/CreateWorkflow/VerifyCommit/styles.ts

@@ -149,5 +149,10 @@ const useStyles = makeStyles((theme: Theme) => ({
  buttomPad: {
    paddingBottom: theme.spacing(3.75),
  },
+  closeBtn: {
+    color: theme.palette.secondary.contrastText,


It's better to raise the a PR to litmus-ui and avoid unwanted coding practices.
Can you track this change and make sure to remove it after updating litmus-ui @ishangupta-ds

ishangupta-ds · 2021-03-12T16:54:26Z

The close button is at a good place for all other use cases but for the editor it interferes with the text area, I am trying an alternative fix. @arkajyotiMukherjee

Signed-off-by: ishangupta-ds <[email protected]>

ishangupta-ds · 2021-03-12T17:01:42Z

suggested changes have been made @arkajyotiMukherjee PTAL.

Changes made.

arkajyotiMukherjee · 2021-03-12T17:18:12Z

Let's still create an issue in litmus-ui and link in this PR thread...if not relevant we will close it @ishangupta-ds

ishangupta-ds · 2021-03-12T17:27:56Z

Here is the issue @arkajyotiMukherjee litmuschaos/litmus-ui#28 (comment)

ishangupta-ds · 2021-03-12T17:29:42Z

@Saranya-jena PTAL at the settings bug fixes.

vanshBhatia-A4k9

Thank you for the changes 🙂

ishangupta-ds · 2021-03-12T17:46:52Z

@rajdas98 we may merge this now before beta 1

ishangupta-ds · 2021-03-12T17:47:38Z

/merge

Signed-off-by: ishangupta-ds <[email protected]>

…to beta-fix

ishangupta-ds added 3 commits March 12, 2021 17:39

bug fixes for beta 1

fdd200a

Signed-off-by: ishangupta-ds <[email protected]>

modal bug fixes for beta 1

07eefa8

Signed-off-by: ishangupta-ds <[email protected]>

Merge remote-tracking branch 'upstream/master' into beta-fix

67ea470

ishangupta-ds added area/litmus-portal kind/bug-fix labels Mar 12, 2021

ishangupta-ds requested review from whitetiger1399, imrajdas, Saranya-jena, vanshBhatia-A4k9, amityt, Jonsy13 and SarthakJain26 March 12, 2021 12:43

ishangupta-ds self-assigned this Mar 12, 2021

Reverting changed in lock file.

d8c9b14

Signed-off-by: ishangupta-ds <[email protected]>

ishangupta-ds added the state/review-in-progress label Mar 12, 2021

whitetiger1399 reviewed Mar 12, 2021

View reviewed changes

vanshBhatia-A4k9 requested changes Mar 12, 2021

View reviewed changes

litmus-portal/frontend/src/views/Community/GeoMap/styles.ts Show resolved Hide resolved

codesniffy bot added the PR: Changes Requested label Mar 12, 2021

whitetiger1399 reviewed Mar 12, 2021

View reviewed changes

vanshBhatia-A4k9 previously requested changes Mar 12, 2021

View reviewed changes

litmus-portal/frontend/src/views/Community/CommunityTimeSeriesPlot/styles.ts Show resolved Hide resolved

whitetiger1399 reviewed Mar 12, 2021

View reviewed changes

arkajyotiMukherjee previously requested changes Mar 12, 2021

View reviewed changes

Minor fix

91a3a32

Signed-off-by: ishangupta-ds <[email protected]>

ishangupta-ds requested a review from arkajyotiMukherjee March 12, 2021 17:10

ishangupta-ds added state/review-in-progress and removed PR: Changes Requested state/review-complete labels Mar 12, 2021

arkajyotiMukherjee previously approved these changes Mar 12, 2021

View reviewed changes

ishangupta-ds added state/review-complete and removed state/review-in-progress labels Mar 12, 2021

vanshBhatia-A4k9 previously approved these changes Mar 12, 2021

View reviewed changes

github-actions bot added the merge label Mar 12, 2021

Minor fix

4c3da95

Signed-off-by: ishangupta-ds <[email protected]>

ishangupta-ds dismissed stale reviews from vanshBhatia-A4k9 and arkajyotiMukherjee via 4c3da95 March 13, 2021 05:43

ishangupta-ds requested review from vanshBhatia-A4k9 and arkajyotiMukherjee March 13, 2021 05:47

ishangupta-ds added the READY TO MERGE label Mar 13, 2021

imrajdas and others added 3 commits March 13, 2021 11:43

Merge branch 'master' into beta-fix

f19fa39

Minor fix

f9557d6

Signed-off-by: ishangupta-ds <[email protected]>

Merge branch 'beta-fix' of https://github.com/ishangupta-ds/litmus in…

900b4f6

…to beta-fix

imrajdas merged commit 22f1306 into litmuschaos:master Mar 13, 2021

CSS and other bug fixes for 2.0 beta 1 #2535

CSS and other bug fixes for 2.0 beta 1 #2535

Conversation

ishangupta-ds commented Mar 12, 2021

Proposed changes

Types of changes

Checklist

Special notes for your reviewer:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whitetiger1399 Mar 12, 2021 • edited Loading

Choose a reason for hiding this comment

ishangupta-ds Mar 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whitetiger1399 Mar 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ishangupta-ds commented Mar 12, 2021

vanshBhatia-A4k9 commented Mar 12, 2021

ishangupta-ds commented Mar 12, 2021 • edited Loading

vanshBhatia-A4k9 commented Mar 12, 2021

vanshBhatia-A4k9 commented Mar 12, 2021

arkajyotiMukherjee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ishangupta-ds commented Mar 12, 2021

ishangupta-ds commented Mar 12, 2021

arkajyotiMukherjee commented Mar 12, 2021

ishangupta-ds commented Mar 12, 2021

ishangupta-ds commented Mar 12, 2021

vanshBhatia-A4k9 left a comment

Choose a reason for hiding this comment

ishangupta-ds commented Mar 12, 2021

ishangupta-ds commented Mar 12, 2021

whitetiger1399 Mar 12, 2021 •

edited

Loading

ishangupta-ds Mar 12, 2021 •

edited

Loading

whitetiger1399 Mar 12, 2021 •

edited

Loading

ishangupta-ds commented Mar 12, 2021 •

edited

Loading