search

opensearch-project / OpenSearch-Dashboards

📊 Open source visualization dashboards for OpenSearch.
https://opensearch.org/docs/latest/dashboards/index/
Apache License 2.0
1.66k stars 868 forks source link

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

Open curq opened 1 year ago

curq commented 1 year ago

Audit for https://github.com/opensearch-project/OpenSearch-Dashboards/issues/4099

In this plugin there is only one .scss file with only 4 css classes. https://github.com/opensearch-project/OpenSearch-Dashboards/blob/bd7d70731eb56372aa2a360e85207a1e6396091f/src/plugins/vis_type_table/public/components/table_vis_app.scss#L1-L29

  1. .visTable. A class for the div container of Data Table. https://github.com/opensearch-project/OpenSearch-Dashboards/blob/bd7d70731eb56372aa2a360e85207a1e6396091f/src/plugins/vis_type_table/public/components/table_vis_app.tsx#L46-L64
  2. .visTable__group. Class for the container of each separate table, applied in case there are several tables.https://github.com/opensearch-project/OpenSearch-Dashboards/blob/bd7d70731eb56372aa2a360e85207a1e6396091f/src/plugins/vis_type_table/public/components/table_vis_component_group.tsx#L25-L34 Screenshot 2023-05-29 at 18 47 20
  3. .visTablecomponenttitle. 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. (https://github.com/opensearch-project/OpenSearch-Dashboards/issues/4122, https://github.com/opensearch-project/OpenSearch-Dashboards/issues/3966). The issue for this was already made https://github.com/opensearch-project/oui/issues/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: https://github.com/opensearch-project/oui/issues/774.
joshuarrrr commented 1 year ago
  1. Instead of custom style .visTablecomponenttitle 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 .visTablegroupInColumns 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 .visTablegroupInColumns 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 commented 1 year ago

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