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 the vis_type_table plugin #4162

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

[Research] [CCI] OUI usage audit in the vis_type_table plugin #4162

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

Comments

@curq
Copy link
Collaborator

curq commented May 29, 2023

Audit for #4099

In this plugin there is only one .scss file with only 4 css classes.

// Container for the Table Visualization component
.visTable {
display: flex;
flex-direction: column;
flex: 1 0 0;
overflow: auto;
@include euiScrollBar;
}
// Group container for table visualization components
.visTable__group {
padding: $euiSizeS;
margin-bottom: $euiSizeL;
display: flex;
flex-direction: column;
flex: 0 0 auto;
}
// Style for table component title
.visTable__component__title {
text-align: center;
}
// Modifier for visTables__group when displayed in columns
.visTable__groupInColumns {
flex-direction: row;
align-items: flex-start;
}


  1. .visTable. A class for the div container of Data Table.
    <I18nProvider>
    <OpenSearchDashboardsContextProvider services={services}>
    <div className={className} data-test-subj="visTable">
    {table ? (
    <TableVisComponent
    table={table}
    visConfig={visConfig}
    event={handlers.event}
    uiState={tableUiState}
    />
    ) : (
    <TableVisComponentGroup
    tableGroups={tableGroups}
    visConfig={visConfig}
    event={handlers.event}
    uiState={tableUiState}
    />
    )}
    </div>
  2. .visTable__group. Class for the container of each separate table, applied in case there are several tables.
    {tableGroups.map(({ table, title }) => (
    <div key={title} className="visTable__group">
    <TableVisComponent
    title={title}
    table={table}
    visConfig={visConfig}
    event={event}
    uiState={uiState}
    />
    </div>
    Screenshot 2023-05-29 at 18 47 20
  3. .visTable__component__title. Used to center individual table's title.
  4. .visTable__groupInColumns. Used to change the direction of visTable container, so instead of multiple table in one column, there will be multiple table in one row.
Screenshot 2023-05-29 at 19 09 47 1

Conclusion

After looking through the plugin files, it seems like overall it is mostly using OUI with almost no native html. The only exceptions are the div containers for that use visTable__group and visTable custom styles. There are other usage of native html, but they are expected ( e.g. h3 in EuiTitle ).

Also, the flex: x y z style is used in both containers. Currently OUI flex component do not support flex-shrink and flex-basis. The same observations were made in other OUI audits. (#4122, #3966). The issue for this was already made opensearch-project/oui#776.

Suggestions.

  1. Instead of custom style .visTable__component__title for centering text, use already existing OUI utility class oui-textCenter.
  2. Replace div containers with OUI flex components. It will also allow us to get rid of the .visTable__groupInColumns custom styling, because the OuiFlexGroup and OuiFlexItem do support both flex-direction and align-items. Current solution uses classnames to dynamically change the direction by applying .visTable__groupInColumns class to .visTable div container.
  3. It is also looks like overflow property is a good candidate for utility class as it is used in several places in OSD. The issue was opened: Add oui-overflow* CSS utilities oui#774.
@curq curq added the enhancement New feature or request label May 29, 2023
@BSFishy BSFishy added OUI Issues that require migration to OUI and removed untriaged labels May 29, 2023
@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

  1. Instead of custom style .visTable__component__title for centering text, use already existing OUI utility class oui-textCenter.

agree 💯

  1. Replace div containers with OUI flex components. It will also allow us to get rid of the .visTable__groupInColumns custom styling, because the OuiFlexGroup and OuiFlexItem do support both flex-direction and align-items. Current solution uses classnames to dynamically change the direction by applying .visTable__groupInColumns class to .visTable div container.

agree - can you create an issue for this change? An additional enhancement I'd love to see here is to add an additional option to wrap items, either by column or row. I'll open a separate issue for it, because it will be easier to do once it's refactored to use OuiFlex* components.

@joshuarrrr
Copy link
Member

Leaving open for now until we complete #4255 and see what additional recommendations we may need.

@joshuarrrr joshuarrrr changed the title [Research] [CCI] OUI usage audit in the vist_type_table plugin [Research] [CCI] OUI usage audit in the vis_type_table plugin Jun 8, 2023
@joshuarrrr joshuarrrr changed the title [Research] [CCI] OUI usage audit in the vis_type_table plugin [Research] [CCI] OUI usage audit in the vis_type_table plugin Jun 8, 2023
@joshuarrrr joshuarrrr moved this to In Progress in Look & Feel Jun 8, 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 OUI Issues that require migration to OUI
Projects
Status: In Progress
Development

No branches or pull requests

3 participants