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 Compliance audit of src/core/public/styles #4167

Open
curq opened this issue May 30, 2023 · 0 comments
Open

[Research] [CCI] OUI Compliance audit of src/core/public/styles #4167

curq opened this issue May 30, 2023 · 0 comments
Assignees
Labels
enhancement New feature or request OUI compliance Issues and PRs to maximize OUI usage and remove style and component hacks

Comments

@curq
Copy link
Collaborator

curq commented May 30, 2023

Audit for #4117

There are only 3 .scss files in this folder, with one of them being just _index.scss that includes the rest of two

  1. _ace_overrides.scss consist of styles for Ace Code Editor, and therefore does not need any change.
  2. _base.scss Mostly there are styles that support or supplement OUI, like
    // We apply brute force focus states to anything not coming from Eui
    // which has focus states designed at the component level.
    // You can also use "osd-resetFocusState" to not apply the default focus
    // state. This is useful when you've already hand crafted your own
    // focus states in OpenSearch Dashboards.
    :focus {
    &:not([class^="eui"]):not(.osd-resetFocusState) {
    @include euiFocusRing;
    }
    }

In _base.scss there two other classes unrelated to OUI: .application and .app-container

.application,
.app-container {
> * {
position: relative;
}
}
.application {
position: relative;
z-index: 0;
display: flex;
flex: 1 0 auto;
flex-direction: column;
> * {
flex-shrink: 0;
}
}

  1. .app-container is used in plain html documents and therefore is not replacable by OUI components. e.g.
    <dashboard-app
    class="app-container dshAppContainer"
    ng-class="{'dshAppContainer--withMargins': model.useMargins}"
    >
    <div id="dashboardChrome"></div>
    <h1 class="euiScreenReaderOnly">{{screenTitle}}</h1>
    <div id="dashboardViewport"></div>
    </dashboard-app>

    It is also used in visualize plugin VisualizeEditorCommon component.
    return (
    <div className={`app-container visEditor visEditor--${visInstance?.vis.type.name}`}>
    {visInstance && appState && currentAppState && (
    <VisualizeTopNav
    currentAppState={currentAppState}

    I'm not sure why this class is used there instead of using local css classes.
  2. .application is used for the OSD app container and testing.
image https://github.com/opensearch-project/OpenSearch-Dashboards/blob/bd7d70731eb56372aa2a360e85207a1e6396091f/src/core/public/rendering/app_containers.tsx#L47-L52 It looks like it is fine as it is and doesn't need change.

Conclusion

Overall it seems that nothing needs to be changed in src/core/public/styles, but it might be better to remove usage of .app-container style in visualize plugin in favor of local css classes.

@curq curq added the enhancement New feature or request label May 30, 2023
@ananzh ananzh removed the untriaged label May 30, 2023
@joshuarrrr joshuarrrr added the OUI compliance Issues and PRs to maximize OUI usage and remove style and component hacks label Jun 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request OUI compliance Issues and PRs to maximize OUI usage and remove style and component hacks
Projects
None yet
Development

No branches or pull requests

3 participants