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

[Research] [CCI] OUI Usage Audit in console plugin #3966

Open
curq opened this issue May 2, 2023 · 6 comments
Open

[Research] [CCI] OUI Usage Audit in console plugin #3966

curq opened this issue May 2, 2023 · 6 comments
Assignees
Labels
OUI compliance Issues and PRs to maximize OUI usage and remove style and component hacks OUI Issues that require migration to OUI research

Comments

@curq
Copy link
Collaborator

curq commented May 2, 2023

I have preformed audit of the console plugin. To start I identified all .scss files and found that custom styling is used in 3 of them (the rest are files with only imports). They are _app.scss, _history.scss and _help.scss. To start, I decided to look through every custom style and investigate what they do.


_app.scss

  • #consoleRoot, .conApp, .conApp__editor, .conApp__output, .conApp__editorContent, .conApp__outputContent These are styling for the <div> containers with flex that should be replaceable by OUI flex components.
  • .conApp__autoComplete Is a classname that used by single <ul> element that looks like is no longer used. The same thing was mentioned in comments.
    // SASSTODO: This component seems to not be used anymore?
    // Possibly replaced by the Ace version
    .conApp__autoComplete {
    position: absolute;
    left: -1000px;
    visibility: hidden;
    /* by pass any other element in ace and resize bar, but not modal popups */
    z-index: $euiZLevel1 + 2;
    margin-top: 22px;
    }
  • .conApp__editorActions, .conApp__editorActionButton, .conApp__editorActionButton--success Used for styling of action buttons. The <button> was used insted of OUI components. If replaced less styling will likely be used.
  • .conApp__resizer This is classname for the resizer element (implemented in <button> tag) that dynamically changes size of editor and output fields. Not likely to be removable, unless the separate OUI component will be implement later.
  • .conApp__settingsModal Sets min-width: 460px for the EuiModal. This component does accept prop for max-width, but there is no prop for min-width. Might be reasonable to add it to the OuiModal implementation.
  • .conApp__requestProgressBarContainer position styling for the progress bar.
  • .conApp__tabsExtension sets border-bottom: $euiBorderThin;

_history.scss

All stylings are either minor adjustments like overflow: auto;, color: $euiColorMediumShade; or styling for the html elements used as flex container. Some of them can be removed by using OUI components instead.


_help.scss

  • .cconHelp__example used for styling example code block <div> container.

After analyzing css styles, I investigated usage of native html tags.

  • In a lot of places <div> elements were used as container instead of OUI components.
  • In some files plain <button> were used, instead of OuiButton
  • Many html elements were used as part of OUI components like OuiText OuiScreenReaderOnly, where raw html is expected.
  • <ul>, <li> tags used as a container in console/application/containers/editor/legacy/editor.tsx We could possibly replace them with OUI container or list components

Conclusion

A large chunk of the custom styling are aimed to style native html elements, especially containers that use flex. So converting them to OUI will reduce number of custom stylings. Furthermore most if not all native html elements can be converted to OUI, with exception of places where the raw html is expected by the OUI components.
Files with native html elements in console/public/application/ (except when used appropriately with OUI):

  • ./components/: console_menu, editor_content_spinner, editor_example
  • ./containers/console_history: console_history, history_viewer
  • ./containers/main: main
  • ./containers/editor/legacy/console_editor: editor_output, editor
  • ./containers/editor: editor

Suggestions

Based on my finding I have a few suggestions.

  • Currently OuiFlexItem only have props for flex grow, and not for shrink and basis. While looking through custom styles I noticed that flex: x y z; is often used, so adding support for it might be worth it.
  • As far as I know resizer used in several other places in Dashboards like Data Table Visualization, and it looks like we have OUI component for that OuiResizableContainer, but we don't utilize it here and instead custom component PanelsContainer from opensearch_dashboards_react/public was used. I believe we should switch from it to OUI component.
    <PanelsContainer onPanelWidthChange={onPanelWidthChange} resizerClassName="conApp__resizer">
    <Panel
    style={{ height: '100%', position: 'relative', minWidth: PANEL_MIN_WIDTH }}
    initialWidth={firstPanelWidth}
    >
    {loading ? (
    <EditorContentSpinner />
    ) : (
    <EditorUI initialTextValue={currentTextObject.text} dataSourceId={dataSourceId} />
    )}
    </Panel>
    <Panel
    style={{ height: '100%', position: 'relative', minWidth: PANEL_MIN_WIDTH }}
    initialWidth={secondPanelWidth}
    >
    {loading ? <EditorContentSpinner /> : <EditorOutput />}
    </Panel>
    </PanelsContainer>
  • Add minWidth option for the OuiModal component. Currently it only got maxWidth prop, so we have to use custom styling to set min-width css property.

Action Plan

  1. Replace native html elements with OUI components, update tests if applicable.
  2. Review the custom styling and remove unnecessary rules
  3. Report any additional finding or issues.
@BSFishy
Copy link
Contributor

BSFishy commented May 25, 2023

Currently OuiFlexItem only have props for flex grow, and not for shrink and basis. While looking through custom styles I noticed that flex: x y z; is often used, so adding support for it might be worth it.

opensearch-project/oui#776 (for tracking :) )

Add minWidth option for the OuiModal component. Currently it only got maxWidth prop, so we have to use custom styling to set min-width css property.

Do you want to create a feature proposal for this in OUI? If not, I can

@curq
Copy link
Collaborator Author

curq commented May 25, 2023

@BSFishy Yes, I created a feature proposal opensearch-project/oui#783

@joshuarrrr joshuarrrr added the OUI compliance Issues and PRs to maximize OUI usage and remove style and component hacks label Jun 1, 2023
@joshuarrrr
Copy link
Member

@curq Can you add some screenshots to this audit so that it's easier to understand visually and functionally what some of these components and styles are doing?

@joshuarrrr
Copy link
Member

At a high level, it may also be worth analyzing this component in the context of #4160, because dev_tools is the wrapper for this functionality, and fixing it's layout to utilize https://oui.opensearch.org/1.0/#/layout/page may make many of the layout styles here unnecessary. I've also provided some detailed feedback:

#consoleRoot, .conApp, .conApp__editor, .conApp__output, .conApp__editorContent, .conApp__outputContent These are styling for the

containers with flex that should be replaceable by OUI flex components.

Yeah, that's one valid way to go, but I'd wait on #4160, first.

.conApp__autoComplete Is a classname that used by single

    element that looks like is no longer used. The same thing was mentioned in comments.

Yeah, let's remove.

.conApp__editorActions, .conApp__editorActionButton, .conApp__editorActionButton--success Used for styling of action buttons. The was used insted of OUI components. If replaced less styling will likely be used.

Probably OuiButtonIcon for the action buttons.

.conApp__resizer This is classname for the resizer element (implemented in tag) that dynamically changes size of editor and output fields. Not likely to be removable, unless the separate OUI component will be implement later.
As far as I know resizer used in several other places in Dashboards like Data Table Visualization, and it looks like we have OUI component for that OuiResizableContainer, but we don't utilize it here and instead custom component PanelsContainer from opensearch_dashboards_react/public was used. I believe we should switch from it to OUI component.

Agree with this approach. Can you also leave a comment linking to that finding on #4089? It will help whoever picks that up start with the context.

.conApp__requestProgressBarContainer position styling for the progress bar.
.conApp__tabsExtension sets border-bottom: $euiBorderThin;

These are likely antipatterns we can get updated design guidance on once we provide screenshots.

All stylings are either minor adjustments like overflow: auto;, color: $euiColorMediumShade; or styling for the html elements used as flex container. Some of them can be removed by using OUI components instead.

We should aim to remove all these styles. Start by moving to OUI components, and then we can see what styles remain.(For example, overflow can be handled by https://oui.opensearch.org/1.0/#/utilities/css-utility-classes, but it's probably not needed at all).

@joshuarrrr
Copy link
Member

@curq It sounds like you have a good plan. The next step is to create issues for actually implementing these changes. The first issue could either be to use OuiResizableContainer, or else to start by replacing all the low-level native components with OUI equivalents. You can make multiple issues at once, or you can do it incrementally, making one issue and a PR to solve that problem, and then moving on to the next phase.

@curq
Copy link
Collaborator Author

curq commented Jun 9, 2023

@joshuarrrr Thank for the feedback! I will take an incremental approach and do it one by one starting with OuiResizableContainer. During working on this first issue I will make sure to capture screenshots for better visual overview. I will update this audit after that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OUI compliance Issues and PRs to maximize OUI usage and remove style and component hacks OUI Issues that require migration to OUI research
Projects
Status: In Progress
Development

No branches or pull requests

4 participants