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

Reduce Prop Reliance #70

Closed
jolanglinais opened this issue Mar 2, 2020 · 12 comments · Fixed by #107
Closed

Reduce Prop Reliance #70

jolanglinais opened this issue Mar 2, 2020 · 12 comments · Fixed by #107

Comments

@jolanglinais
Copy link
Member

We should not be in the habit of relying on a lot of styling props for individual fields. This practice has gotten out of hand:

ContractEditor
  clauseProps = {
    BODY_FONT (string),
    CLAUSE_BACKGROUND (string),
    CLAUSE_BORDER (string),
    CLAUSE_ICONS (string),
    COMPUTED_COLOR (string),
    HEADER_FONT (string),
    VARIABLE_COLOR (string),
    CONDITIONAL_COLOR (string),
};
  editorProps = {
    BUTTON_BACKGROUND_INACTIVE (string),
    BUTTON_BACKGROUND_ACTIVE (string),
    BUTTON_SYMBOL_INACTIVE (string),
    BUTTON_SYMBOL_ACTIVE (string),
    DROPDOWN_COLOR (string),
    EDITOR_BORDER (string),
    EDITOR_BORDER_RADIUS (string),
    EDITOR_HEIGHT (string),
    EDITOR_MARGIN (string),
    EDITOR_SHADOW (string),
    EDITOR_WIDTH (string),
    TOOLBAR_BACKGROUND (string),
    TOOLTIP_BACKGROUND (string),
    TOOLTIP (string),
    TOOLBAR_SHADOW (string),
};


ErrorLogger
  errorsPropsObject = {
    ERRORS_HEADER_BACKGROUND (string),
    ERRORS_HEADER_BACKGROUND_HOVER (string),
    ERRORS_HEADER_EXPAND_ARROW (string),
    ERRORS_HEADER_BORDER_TOP (string),
    ERRORS_HEADER_SHADOW (string),
    ERRORS_DISPLAY_BACKGROUND (string),
    ERRORS_DISPLAY_SHADOW (string),
    ERRORS_DISPLAY_Z_INDEX (string),
    ERROR_BORDER_BOTTOM (string),
    ERROR_EXPAND_ARROW (string),
    ERROR_FILE (string),
    ERROR_FILE_HOVER (string),
    ERROR_TYPE (string),
    ERROR_FULL_MESSAGE (string),
    ERROR_SHORT_MESSAGE (string),
};


Navigation
  navigationProps = {
    NAVIGATE_SWITCH_TITLE_ACTIVE_COLOR (string),
    NAVIGATE_SWITCH_TITLE_INACTIVE_COLOR (string),
    NAVIGATE_SWITCH_TITLE_FONT_FAMILY (string),
    NAVIGATION_POSITION (string),
    NAVIGATION_TOP_VALUE (string),
    NAVIGATION_MAX_HEIGHT (string),
    NAVIGATION_WIDTH (string),
    NAVIGATION_BACKGROUND_COLOR (string),
    CONTRACT_NAVIGATION_HEADER_COLOR (string),
    CONTRACT_NAVIGATION_CLAUSE_COLOR (string),
    CONTRACT_NAVIGATION_CLAUSE_HEADER_COLOR (string),
    CLAUSE_NAVIGATION_TITLE_COLOR (string),
    CLAUSE_NAVIGATION_TITLE_HOVER_COLOR (string),
    CLAUSE_NAVIGATION_EXPANSION_ARROW_COLOR (string),
    CLAUSE_NAVIGATION_EXPANSION_ARROW_HOVER_COLOR (string),
    CLAUSE_NAVIGATION_FILE_DELETE_COLOR (string),
    CLAUSE_NAVIGATION_FILE_DELETE_HOVER_COLOR (string),
    CLAUSE_NAVIGATION_FILE_ADD_COLOR (string),
    CLAUSE_NAVIGATION_FILE_ADD_HOVER_COLOR (string),
};


TemplateLibrary
  libraryPropsObject = {
    ACTION_BUTTON (string),
    ACTION_BUTTON_BG (string),
    ACTION_BUTTON_BORDER (string),
    HEADER_TITLE (string),
    SEARCH_COLOR (string),
    TEMPLATE_BACKGROUND (string),
    TEMPLATE_DESCRIPTION (string),
    TEMPLATE_TITLE (string),
};

All of these should be removed and we should instead rely on a strong system of structured class names. This will allow users to handle what these props currently do via overriding the CSS.

Related Issue:

@kanav-raina
Copy link

I would like to work on this.
Kindly assign this to me

@jolanglinais
Copy link
Member Author

I think it makes the most sense for the person working on the related issue to do this as well, @98lenvi

@kanav-raina
Copy link

Ok

@98lenvi
Copy link

98lenvi commented Mar 3, 2020

@irmerk

I'll start with markdown editor accordproject/cicero-ui#255, If the approach looks good, then I'll apply the same here as well.

@shakti97
Copy link

Can I work on this issue @irmerk

@jolanglinais
Copy link
Member Author

This issue may be affected by the migration to Slate v0.57, so I'd hold off right now.

@jolanglinais
Copy link
Member Author

@98lenvi @kanav-raina @shakti97:

accordproject/cicero-ui#357 (and accordproject/markdown-editor#297) has been finished, so there is no longer a blocker on this issue. The migration may have solved this issue, so look into that first.

@98lenvi
Copy link

98lenvi commented Apr 13, 2020

@irmerk, Sorry about not responding earlier. I'll check and report my observations here.

@jolanglinais
Copy link
Member Author

I believe the ContractEditor portion of this issue is finished.

@jolanglinais
Copy link
Member Author

We should also keep potential collisions in mind, maybe prepend className with 'ciceroUI' or something.

@jolanglinais jolanglinais transferred this issue from accordproject/cicero-ui May 13, 2020
@jolanglinais jolanglinais pinned this issue May 13, 2020
@jolanglinais
Copy link
Member Author

Code has been started: accordproject/cicero-ui#387

@jolanglinais jolanglinais self-assigned this Jun 2, 2020
@jolanglinais
Copy link
Member Author

jolanglinais commented Jun 2, 2020

  • ContractEditor
    • clauseProps
    • editorProps
  • ErrorLogger
    • errorsPropsObject
  • Navigation
    • navigationProps
  • TemplateLibrary
    • libraryPropsObject

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants