Feature/refactoring project settings #476

karimould · 2021-11-05T12:18:25Z

For new features:

Discuss feature: Material-UI 🙏🏻 Help Needed: boldui -> Material-UI #401
Describe the use-case in details: Migrated ProjectSettings and Hook editor to MUI
Ensure the solution is backwards-compatible
Test it. Submit screenshots / outputs / tests together with the PR.

… into feature/refactoring-projectSettings

agoldis

Thank you very much @karimould! That's a very valuable contribution, it brings us really close to wrapping up v2 release.

I have a few suggestions, please tell if you think those make sense:

This one is regarding all the input fields:

Also, here's a suggestion that never was implemented - should we add a confirmation dialog before deleting a webhook?

agoldis · 2021-11-06T06:20:40Z

@ImanMahmoudinasab feel free to tune in

ImanMahmoudinasab · 2021-11-07T20:19:41Z

@karimould @agoldis What I had in mind to implement the hooks was something like this:

Having separate menu item for integrations
Paper for every hook added by user
Add button on top of page that opens a menu showing different types of hooks that user can add

ImanMahmoudinasab · 2021-11-07T20:24:15Z

@agoldis Sticking to what @karimould has implemented, I think addressing your comments are enough to merge the pr. In the feature, @karimould or I will refactor it.

added more spacing to the form added a modal for delete conformation before deleting a hook alignt info icon with the label text

karimould · 2021-11-07T20:41:19Z

@ImanMahmoudinasab this looks very good, i can take care of it :-)

@agoldis
I have implemented the changes, with the exception of the better separation of the hooks in the unfolded state.

I would suggest that if the code is okay, the current version could be merged.
I would then take care of moving the hooks to 'intergrations' in a later refactoring :-)

ImanMahmoudinasab

@karimould Thank you for the migration. I put some comments for improving the code style.

ImanMahmoudinasab · 2021-11-08T01:46:35Z

packages/dashboard/src/components/common/textFieldLabel.tsx

 import React, { PropsWithChildren } from 'react';

 interface InputFieldLabelProps extends FormControlProps {
  helpText?: string;
+  htmlFor: string;
+  label?: string | React.ReactNode;


Suggested change

label?: string | React.ReactNode;

label?: string | ReactNode;

ImanMahmoudinasab · 2021-11-08T01:52:13Z

packages/dashboard/src/components/common/textFieldLabel.tsx

+  const hasError = errorMessage ? true : false;
+  const Help = helpText && (
+    <Tooltip title={helpText}>
+      <InfoOutlinedIcon fontSize="small" sx={{ marginRight: '0.25rem' }} />


MUI recommends its spacing concept, for margins and paddings. Every space is 8px by default (can be changed in the theme options). So if you need 4px space you can use:

Suggested change

<InfoOutlinedIcon fontSize="small" sx={{ marginRight: '0.25rem' }} />

<InfoOutlinedIcon fontSize="small" sx={{ mr: 0.5 }} />

ImanMahmoudinasab · 2021-11-08T02:07:30Z

packages/dashboard/src/project/deleteProject.tsx

+          <Button
+            variant="contained"
+            color="error"
+            onClick={() => setShowModal(true)}
+            disabled={deleting}
+          >
+            <DeleteIcon />
+            <Typography variant="body1">
+              {deleting ? 'Deleting' : 'Delete'}
+            </Typography>
+          </Button>


Suggested change

<Button

variant="contained"

color="error"

onClick={() => setShowModal(true)}

disabled={deleting}

>

<DeleteIcon />

<Typography variant="body1">

{deleting ? 'Deleting' : 'Delete'}

</Typography>

</Button>

<Button

variant="contained"

color="error"

onClick={() => setShowModal(true)}

startIcon={<DeleteIcon />

disabled={deleting}

>

{deleting ? 'Deleting' : 'Delete'}

</Button>

This way icon and text will be aligned.
You don't need Typography. all icons should have same typography applied by the button itself.

ImanMahmoudinasab · 2021-11-08T02:09:09Z

packages/dashboard/src/project/deleteProject.tsx

+            onClick={deleteProject}
+            disabled={deleting}
+          >
+            <DeleteIcon />


The same thing here, use startIcon prop.

ImanMahmoudinasab · 2021-11-08T02:11:03Z

packages/dashboard/src/project/hook/bitbucketHook.tsx

-        </Cell>
-        <Cell>
+        </Grid>
+        <Grid>


Suggested change

<Grid>

<Grid item>

ImanMahmoudinasab · 2021-11-08T02:14:27Z

packages/dashboard/src/project/hook/editHookEvents.tsx

  return (
-    <Cell xs={12}>
+    <Grid item xs={12}>


All <Grid item>s should be wrapped with <Grid container>s.

ImanMahmoudinasab · 2021-11-08T02:15:14Z

packages/dashboard/src/project/hook/genericHook.tsx

@@ -40,13 +40,13 @@ export const GenericHook = ({ hook }: GenericHookProps) => {
    <FormProvider {...formMethods}>
      <form onSubmit={handleSubmit(onSubmit)}>
        <Grid>


No description provided.

ImanMahmoudinasab · 2021-11-08T02:18:24Z

packages/dashboard/src/project/hook/hookEdit.tsx

+    <Box
+      style={{ display: 'flex', cursor: 'pointer' }}
+      onClick={() => toggleExpanded()}
+      alignItems="center"
+    >


Suggested change

<Box

style={{ display: 'flex', cursor: 'pointer' }}

onClick={() => toggleExpanded()}

alignItems="center"

>

<Box

display="'flex"

cursor="pointer"

onClick={() => toggleExpanded()}

alignItems="center"

>

Done, the cursor prop does not exist.
I styled the cursor with the sx prop.

ImanMahmoudinasab · 2021-11-08T02:21:27Z

packages/dashboard/src/project/hook/hookEdit.tsx

    </>
  );
 };

 const HookDetails = ({ hook }: { hook: Hook }) => {
  return (
-    <Grid style={{ padding: '1rem' }}>
-      <Cell xs={12}>
+    <Grid sx={{ marginTop: '1rem' }}>


Suggested change

<Grid sx={{ marginTop: '1rem' }}>

<Grid container mt={2}>

You don't need sx for styling components (Grid, Box, ... ).
use spacing instead of rem. (2 is 16px set what you need here)

agoldis · 2021-11-08T08:10:05Z

@karimould @ImanMahmoudinasab I am fine with improving the code later, and thanks for implementing the visual changes. Please let me know if you want to tackle Iman's changes within this iteration.

… into feature/refactoring-projectSettings

changed spacing to mui suggested spacing model

karimould added 3 commits October 17, 2021 14:45

WIP

bc6d33d

Merge branch 'master' of https://github.com/sorry-cypress/sorry-cypress…

38988d4

… into feature/refactoring-projectSettings

migrated project settings to mui

a633874

agoldis reviewed Nov 6, 2021

View reviewed changes

adjustments from the code review

cfb29e8

added more spacing to the form added a modal for delete conformation before deleting a hook alignt info icon with the label text

ImanMahmoudinasab reviewed Nov 8, 2021

View reviewed changes

karimould added 2 commits November 9, 2021 17:31

Merge branch 'master' of https://github.com/sorry-cypress/sorry-cypress…

05ba3fd

… into feature/refactoring-projectSettings

changes for review

610c1b0

changed spacing to mui suggested spacing model

ImanMahmoudinasab approved these changes Nov 10, 2021

View reviewed changes

agoldis merged commit 36bffb8 into sorry-cypress:master Nov 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/refactoring project settings #476

Feature/refactoring project settings #476

karimould commented Nov 5, 2021

agoldis left a comment

agoldis commented Nov 6, 2021

ImanMahmoudinasab commented Nov 7, 2021

ImanMahmoudinasab commented Nov 7, 2021

karimould commented Nov 7, 2021

ImanMahmoudinasab left a comment

ImanMahmoudinasab Nov 8, 2021

karimould Nov 9, 2021

ImanMahmoudinasab Nov 8, 2021

karimould Nov 9, 2021

ImanMahmoudinasab Nov 8, 2021

karimould Nov 9, 2021

ImanMahmoudinasab Nov 8, 2021

karimould Nov 9, 2021

ImanMahmoudinasab Nov 8, 2021

karimould Nov 9, 2021

ImanMahmoudinasab Nov 8, 2021

karimould Nov 9, 2021

ImanMahmoudinasab Nov 8, 2021

ImanMahmoudinasab Nov 8, 2021

karimould Nov 9, 2021

ImanMahmoudinasab Nov 8, 2021

karimould Nov 9, 2021

agoldis commented Nov 8, 2021

	label?: string \| React.ReactNode;
	label?: string \| ReactNode;

	<InfoOutlinedIcon fontSize="small" sx={{ marginRight: '0.25rem' }} />
	<InfoOutlinedIcon fontSize="small" sx={{ mr: 0.5 }} />

Feature/refactoring project settings #476

Feature/refactoring project settings #476

Conversation

karimould commented Nov 5, 2021

agoldis left a comment

Choose a reason for hiding this comment

agoldis commented Nov 6, 2021

ImanMahmoudinasab commented Nov 7, 2021

ImanMahmoudinasab commented Nov 7, 2021

karimould commented Nov 7, 2021

ImanMahmoudinasab 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

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

agoldis commented Nov 8, 2021