refactor(*): replace style props with CSS classNames - #320 #387

98lenvi · 2020-04-17T16:58:07Z

Signed-off-by: Lenvin Gonsalves lenvin@oykuapp.com

Issue #320

The goal is to remove all style props in favor of CSS class names.
One pro I can think of is the user will have more flexibility on the appearance of the component,

Changes

The following will be done for each component
- Removing the reliance on user passed props for styling in the styled-components.
- Not passing these styling props down.
- Removing them from propTypes as well.
- Creating a new file customStyles.js which will have the classNames of the components.
- The above file will export a styled-component div which will wrap around the main component.
Will be done for the following components
ErrorLogger
ContractEditor (already done during slate migration to 0.57)
Navigation
TemplateLibrary

Flags

I have done it for the ErrorLogger component, Once the maintainers feel the approach is right, I will perform the above for the other components as well.
Documentation must be updated.
Storybook must be updated.
Tests must be updated. Currently, snapshots are causing tests to fail.

Related Issues

Issue #320

98lenvi · 2020-04-18T14:49:05Z

@arteevraina. Tagging you here so you can get notified when this PR gets merged.

jolanglinais

Left some comments and questions.

jolanglinais · 2020-04-20T14:47:24Z

src/ErrorLogger/Error.js

  };

  const shortMessageProps = {
    onClick: handleClickSpecError,
-    shortMessage: errorProps.ERROR_SHORT_MESSAGE,
  };

  const fullMessageProps = {


fullMessageProps isn't needed anymore.

Thank you for the feedback

jolanglinais · 2020-04-20T14:48:37Z

src/ErrorLogger/Error.js

  };

  return (
-    <SC.ErrorComponent {...componentProps}>
+    <SC.ErrorComponent className={'errorComponent'}>


Do all these need to be className="..." instead of className={'...'}?

Yes, they need to be className="...". I'll change them!

jolanglinais · 2020-04-20T14:49:16Z

src/ErrorLogger/customStyles.js

+import styled from 'styled-components';
+
+export const CustomStylesWrapper = styled.div`
+.errorHeader{
+};
+.errorDisplay{
+};
+.errorBarArrow{
+};
+.errorComponent{
+};
+.errorFile{
+};
+.errorArrowDivExpanded{
+};
+.errorArrowDivCollapsed{
+};
+.errorType{
+};
+.errorShortMessage{
+};
+.errorFullMessage{
+};
+`


Can you explain more why these custom style wrappers help / what they do?

When I first added them they meant sense to me as it is one of the best way for me to added custom CSS style to elements. But now it doesn't seem required at all since the user will be adding his/her own wrapper around the component or style document. Sorry for the inconvenience, I'll be removing it!

jolanglinais · 2020-04-20T14:50:21Z

src/ErrorLogger/styles.js

-    z-index: ${props => props.zIndexInput || 'auto'};
+    background-color: #1E2D53;
+    box-shadow: 0 -2px 20px 0 rgba(20,31,60,0.65);
+    z-index: auto;


auto is the default value for z-index, so these can be removed.

Thank you for the feedback

jolanglinais · 2020-04-20T14:52:14Z

src/Navigation/styles.js

-    width: ${props => props.navWidth || 'inherit'};
-    background-color: ${props => props.backgroundColor || 'inherit'};
+    position: static;
+    top: auto;


auto is the default value for top, so this can be removed.

Thank you for the feedback

DianaLease

Can we also give the highest parent of each component a common class name? maybe ciceroUI? That way we can easily target specific elements within the ciceroUI parent.

I'd also like an explanation of the CustomStylesWrapper that you've added for each component. Thanks!

98lenvi · 2020-04-24T18:15:12Z

Can we also give the highest parent of each component a common class name? maybe ciceroUI? That way we can easily target specific elements within the ciceroUI parent.

I'd also like an explanation of the CustomStylesWrapper that you've added for each component. Thanks!

@DianaLease
Yes I'll work on adding a common className. Thank you for the feedback

When I first added them they meant sense to me as it is one of the best way for me to added custom CSS style to elements. But now it doesn't seem required at all since the user will be adding his/her own wrapper around the component or style document. Sorry for the inconvenience, I'll be removing it!

jolanglinais · 2020-04-24T20:44:43Z

src/TemplateLibrary/index.js


  return (
-      <TemplatesWrapper>
+      <TemplatesWrapper className='ciceroUI'>


Shouldn't this class name be more specific?

Keeping @DianaLease comment in mind, do I need to add specific class names?

I think @DianaLease is correct, so this could be left.

src/Navigation/index.js

DianaLease · 2020-04-24T20:52:49Z

@irmerk I think it makes sense to have a common className at the top level for all cicero-ui components. Like how semantic-ui adds ui as a class name to their parent components. That way when we apply styles we can limit those styles to elements within the .ciceroUI hierarchy.

jolanglinais

Made some different suggestions to keep the className more consistent.

jolanglinais · 2020-04-28T17:41:23Z

src/TemplateLibrary/index.js

          {props.addTemp
          && <NewClauseComponent addTempInput={props.addTemp} />}
        </Functionality>
-        <TemplateCards tempsHeight={libraryProps.TEMPLATES_HEIGHT} >
+        <TemplateCards className='templateCardsDiv' >


Maybe this could be 'templateLibraryCardsWrapper'

src/TemplateLibrary/index.js

jolanglinais · 2020-04-28T17:42:09Z

src/TemplateLibrary/index.js


  return (
-      <TemplatesWrapper>
+      <TemplatesWrapper className='ciceroUI'>


I think @DianaLease is correct, so this could be left.

src/TemplateLibrary/Components/TemplateCard.js

src/ErrorLogger/Error.js

src/Navigation/ComponentSwitch.js

jolanglinais · 2020-04-28T17:49:09Z

src/Navigation/ComponentSwitch.js

            NAVIGATION
        </SC.Navigation>
-        <SC.Files {...fileProps}>
+        <SC.Files {...fileProps} className={props.navState === 'FILES'?'navTitleActive':'navTitleInactive'}>


Maybe this could be 'navigationFilesActive' and 'navigationFilesInactive'

jolanglinais · 2020-05-04T14:32:45Z

Following up on this @98lenvi, need any assistance?

98lenvi · 2020-05-04T14:48:48Z

@irmerk, actually my laptop's hard drive died a week ago. I'm waiting for the new drive to arrive (most probably tonight or tomorrow morning). I'll do the changes tomorrow morning!
Thanks for asking!

… - i320 Signed-off-by: Lenvin Gonsalves <lenvin@oykuapp.com>

…- i320 Signed-off-by: Lenvin Gonsalves <lenvin@oykuapp.com>

…ames - i320 Signed-off-by: Lenvin Gonsalves <lenvin@oykuapp.com>

Signed-off-by: Lenvin Gonsalves <lenvin@oykuapp.com>

jolanglinais

A few more remaining changes.

jolanglinais · 2020-05-05T15:27:00Z

src/TemplateLibrary/index.js

          {props.addTemp
          && <NewClauseComponent addTempInput={props.addTemp} />}
        </Functionality>
-        {filtered && filtered.length ? <TemplateCards tempsHeight={libraryProps.TEMPLATES_HEIGHT} >
+        {filtered && filtered.length ? <TemplateCards className={'templateCardsDiv'} >
          {_.sortBy(filtered, ['name']).map(t => (
            <TemplateCard
              key={t.uri}
              addToCont={props.addToCont}
              template={t}
              handleViewTemplate={props.handleViewTemplate}
              className="templateCard"


I believe this className="templateCard" can be removed, it's redundant on the className='templateLibraryCard' in src/TemplateLibrary/Components/TemplateCard.js

jolanglinais · 2020-05-05T15:34:06Z

src/ErrorLogger/Error.js

-      <SC.ArrowDiv {...errorArrowProps} />
-      <SC.ErrorFile {...fileProps} onClick={() => errorNav(error)} >
+      <SC.ArrowDiv {...errorArrowProps} className={specErrorVisible?'errorLoggerErrorArrowExpanded':'errorLoggerErrorArrowCollapsed'}/>
+      <SC.ErrorFile onClick={() => errorNav(error)} className='errorLoggerError'>


'errorLoggerErrorFile'

jolanglinais · 2020-05-05T15:37:07Z

src/Navigation/ComponentSwitch.js

            NAVIGATION
        </SC.Navigation>
-        <SC.Files {...fileProps}>
+        <SC.Files {...fileProps} className={props.navState === 'FILES'?'navigationTitleActive':'navigationTitleInactive'}>


'navigationFilesActive' and 'navigationFilesInactive'

jolanglinais · 2020-05-11T18:55:43Z

Also note the proposed changes in #411

jolanglinais · 2020-05-12T16:37:08Z

We're migrating this repository into web-components the next couple of days, so I'll be closing this soon and you should reopen there.

jolanglinais assigned 98lenvi Apr 17, 2020

jolanglinais added Difficulty: Medium Type: Enhancement ✨ Improvement to process or efficiency labels Apr 17, 2020

jolanglinais requested review from jolanglinais and DianaLease April 17, 2020 19:59

jolanglinais suggested changes Apr 20, 2020

View reviewed changes

DianaLease suggested changes Apr 20, 2020

View reviewed changes

98lenvi force-pushed the 98lenvi/i320/style_props_to_classname branch from 69ad521 to 2c9fd4e Compare April 24, 2020 18:38

98lenvi requested review from DianaLease and jolanglinais April 24, 2020 18:41

jolanglinais reviewed Apr 24, 2020

View reviewed changes

src/Navigation/index.js Show resolved Hide resolved

jolanglinais suggested changes Apr 28, 2020

View reviewed changes

98lenvi added 8 commits May 5, 2020 15:48

refactor(ErrorLogger): remove style props and replace with classNames…

ae0871f

… - i320 Signed-off-by: Lenvin Gonsalves <lenvin@oykuapp.com>

refactor(Navigation): remove style props and replace with classNames …

001b31a

…- i320 Signed-off-by: Lenvin Gonsalves <lenvin@oykuapp.com>

refactor(TemplateLibrary): remove style props and replace with classN…

d1dde36

…ames - i320 Signed-off-by: Lenvin Gonsalves <lenvin@oykuapp.com>

fix(ErrorLogger): Add classNames for for both states of ArrowDiv - i320

3c125e1

Signed-off-by: Lenvin Gonsalves <lenvin@oykuapp.com>

refactor(*): code cleanup,remove unecessary CSS - i320

184a711

Signed-off-by: Lenvin Gonsalves <lenvin@oykuapp.com>

refactor(*): remove CustomStylesWrapper - i320

06bf8ce

Signed-off-by: Lenvin Gonsalves <lenvin@oykuapp.com>

refactor(*): add CiceroUI as common className - i320

b6eabe5

Signed-off-by: Lenvin Gonsalves <lenvin@oykuapp.com>

refactor(*): rename classNames - i320

3b7c7ea

Signed-off-by: Lenvin Gonsalves <lenvin@oykuapp.com>

98lenvi force-pushed the 98lenvi/i320/style_props_to_classname branch from 42a5597 to 3b7c7ea Compare May 5, 2020 10:33

98lenvi requested a review from jolanglinais May 5, 2020 10:34

jolanglinais suggested changes May 5, 2020

View reviewed changes

jolanglinais mentioned this pull request May 13, 2020

Reduce Prop Reliance accordproject/web-components#70

Closed

jolanglinais mentioned this pull request Jun 2, 2020

refactor(cicero-ui): remove prop dependency - I70 accordproject/web-components#107

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(*): replace style props with CSS classNames - #320 #387

refactor(*): replace style props with CSS classNames - #320 #387

98lenvi commented Apr 17, 2020 •

edited

Loading

98lenvi commented Apr 18, 2020

jolanglinais left a comment

jolanglinais Apr 20, 2020

98lenvi Apr 24, 2020

jolanglinais Apr 20, 2020

98lenvi Apr 24, 2020

jolanglinais Apr 20, 2020

98lenvi Apr 24, 2020

jolanglinais Apr 20, 2020

98lenvi Apr 24, 2020

jolanglinais Apr 20, 2020

98lenvi Apr 24, 2020

DianaLease left a comment

98lenvi commented Apr 24, 2020 •

edited

Loading

jolanglinais Apr 24, 2020

98lenvi Apr 25, 2020 •

edited

Loading

jolanglinais Apr 28, 2020

DianaLease commented Apr 24, 2020

jolanglinais left a comment

jolanglinais Apr 28, 2020

jolanglinais Apr 28, 2020

jolanglinais Apr 28, 2020

jolanglinais commented May 4, 2020

98lenvi commented May 4, 2020 •

edited

Loading

jolanglinais left a comment

jolanglinais May 5, 2020

jolanglinais May 5, 2020

jolanglinais May 5, 2020

jolanglinais commented May 11, 2020

jolanglinais commented May 12, 2020

refactor(*): replace style props with CSS classNames - #320 #387

Are you sure you want to change the base?

refactor(*): replace style props with CSS classNames - #320 #387

Conversation

98lenvi commented Apr 17, 2020 • edited Loading

Issue #320

Changes

Flags

Related Issues

98lenvi commented Apr 18, 2020

jolanglinais 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

DianaLease left a comment

Choose a reason for hiding this comment

98lenvi commented Apr 24, 2020 • edited Loading

Choose a reason for hiding this comment

98lenvi Apr 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DianaLease commented Apr 24, 2020

jolanglinais 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

jolanglinais commented May 4, 2020

98lenvi commented May 4, 2020 • edited Loading

jolanglinais 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

jolanglinais commented May 11, 2020

jolanglinais commented May 12, 2020

98lenvi commented Apr 17, 2020 •

edited

Loading

98lenvi commented Apr 24, 2020 •

edited

Loading

98lenvi Apr 25, 2020 •

edited

Loading

98lenvi commented May 4, 2020 •

edited

Loading