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 advanced_settings plugin #3963

Open
SergeyMyssak opened this issue May 2, 2023 · 4 comments
Open

[Research] [CCI] OUI Usage Audit in advanced_settings plugin #3963

SergeyMyssak opened this issue May 2, 2023 · 4 comments
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

@SergeyMyssak
Copy link
Contributor

SergeyMyssak commented May 2, 2023

Audit

I performed the OUI Usage audit to the advanced_settings plugin and got the following results.

This plugin has 8 custom components:

The image below shows all the components listed except advanced_settings_voice_announcement, PageSubtitle and PageFooter, where the text in red is the name of the component.

This plugin also has one scss file - _advanced_settings.scss. The image below shows the use of the classnames written in the file (except .osdBody--mgtAdvancedSettingsHasBottomBar .mgtPage__body), where the text in red is the classname.

Conclusion

  • All 8 components are a combination of existing components and do not represent new common components that can be moved to OUI
  • PageSubtitle and PageFooter are functions that return null and can be removed if they are not to be used in the future
  • scss styles are used for additional placement or to add external effects that do not change the appearance of the components in the OUI
  • .mgtAdvancedSettingsForm__button classname is unnecessary and does not affect anything, so it can be removed
    .mgtAdvancedSettingsForm__button {
    width: 100%;
    }
@SergeyMyssak SergeyMyssak added the enhancement New feature or request label May 2, 2023
@joshuarrrr joshuarrrr added research and removed untriaged enhancement New feature or request labels May 2, 2023
@seanneumann seanneumann added the OUI Issues that require migration to OUI label May 18, 2023
@BSFishy
Copy link
Contributor

BSFishy commented May 25, 2023

All 8 components are a combination of existing components and do not represent new common components that can be moved to OUI

Does that make them candidates for OUI? I'm still not 100% sure on OUI's stance on higher order components. Maybe @KrooshalUX can add some context here

scss styles are used for additional placement or to add external effects that do not change the appearance of the components in the OUI

Does that mark a gap in layouts that OUI provides? Or are the layouts achievable using OUI components?

@KrooshalUX
Copy link

KrooshalUX commented May 25, 2023

Great run down, thanks @SergeyMyssak !

HOCs need to be evaluated and treated in a case-by-case basis (vague, I know...) there really isn't a once-size-fits-all answer here, especially since a lot of context for implementation decisions has been lost over time.

As a next step before I can make any recommendation from UX/OSDS would be to have engineering describe what functionality the higher order components introduce.

For example - lets use the Search HOC - Looking at the comment the EUI component was broken at the time of build, so there was a "hack" introduced to implement incremental search. In later versions of the EuiSearchBar component, that issue has been fixed, therefore (potentially/hopefully) eliminating the need for the HOC.

Screen Shot 2023-05-25 at 11 54 06 AM

@SergeyMyssak
Copy link
Contributor Author

SergeyMyssak commented May 30, 2023

From my point of view, we have to choose carefully which components to move to the OUI and which ones are better left. It is not always advantageous to move components to the component library. I've summarized some of the points we should pay attention to:

  1. Increased complexity: Each component must be generalized enough to fit multiple uses, and extra care must be taken to ensure compatibility with different project parts.
  2. Maintenance overhead: Updates, patches, and other changes must be carefully managed to avoid breaking dependent project parts. Also, if we need to fix or add something, we have to apply it for the OUI first, wait for its release, and then only update the necessary code in Dashboards.
  3. Decoupling difficulties: If components are tightly coupled to specific business logic or other parts of our application, separating them into a component library can be challenging and may result in a loss of functionality or require extensive refactoring.
  4. Over-generalization: In an effort to make components reusable, there's a risk of over-generalization. This can lead to bloated components with unnecessary features, making them harder to understand, maintain, and use.
  5. Performance considerations: More complex and generalized components can lead to less than optimal performance. If a component includes features or functionality that are not needed for a particular use case, it could unnecessarily slow down the application.

Overview of components and their portability to the OUI:

  1. Search

search uses EuiSearchBar component from the OUI library. We also have a second place where the EuiSearchBar is used, it's in the Table component in the saved_objects_management plugin. Their functionality is similar, namely error handling and displaying it.

  • Why was search component created in advanced_settings plugin, but saved_objects_management plugin doesn't?
    I assume search component was created to make it easier to manage EuiSearchBar, in terms of passing the necessary parameters, so as not to show them in the parent component. I think it's a good practice that could also be used in saved_objects_management plugin. This can be considered as an action item.

  • Can we move anything in the OUI?
    In the question above, I described the general functionality used for EuiSearchBar. Since the logic inside EuiSearchBar checks, if the query is correct or not, we could add the functionality to display an error in EuiSearchBar. But, I think this is not a very good idea because the logic inside EuiSearchBar shouldn't know anything about validation of the query and error displaying, we could do this in a new component which would be a wrapper for EuiSearchBar. My suggestion is based on the case that if we need a pure search with no validation of the query or errors displaying, we would not have one because EuiSearchBar would be tied to some logic. Also, we have to consider that OuiSearchBar component is used in in_memory_table in OUI. This component has its own way of displaying errors. By making this functionality optional, we may encounter problems number 1, 3, and 4. Alternatively, we can create a hook in Dashboards that will manage the error state and be used in these two components.

  • [CCI] Standardize error handling for query input in the SearchBar #4182

Screenshot 2023-05-30 at 10 29 52 Screenshot 2023-05-30 at 10 29 42 Screenshot 2023-05-30 at 12 20 00

@KrooshalUX, I have a question about the incremental parameter. Should we remove it? Everything seems to work fine without it.

  1. advanced_settings_voice_announcement

This component is only used in one place and for one purpose, to announce the entered query in the OuiSearchBar. I think we could reuse this wherever OuiSearchBar is used, namely in Search, Table, and in_memory_table. But before we do that, we need to understand if OuiSearchBar actually performs the same function in those components. By making this functionality, we may encounter problems number 1 and 4. As a result, we could create a new component in the OUI and put it in the accessibility folder.

  1. call_outs

This component is unique and was put into a separate component to reduce the amount of code in the parent component. We can leave everything as it is.

  1. field

This component was discussed during office hours. In short, we have two components: field (advanced_settings) and field (saved_objects_management) where we duplicate the logic. Action items we need to discuss:

  • Create a Field component in the OUI or a generic component in Dashboards, and then use it in advanced_settings and saved_objects_management plugins.
  • Make the form in saved_objects_management plugin the same as in advanced_settings plugin to comply with consistency. I think this is where we need help from @KrooshalUX on consulting. I will create a proposal for this, where we can discuss this in more detail.

Screenshot 2023-05-30 at 19 06 27 Screenshot 2023-05-30 at 19 07 15

By making these functionalities, we may encounter problems number 1, 2, 3, and 4.

  1. form

We use this component to compose our fields and the bottom bar. In this component, we use OuiForm component for each category where we map settings which returns the Field component which contains a settings label and description using the OuiDescribedFormGroup component, and field. This component has been described in the 2nd action item above that can be reused either as a separate component in the OUI or as a generic component in Dashboards.


Analysis of css styles:

All the styles that are in the _advanced_settings.scss file are used to style the form component and bottom bar content.

Screenshot 2023-05-30 at 19 35 57 Screenshot 2023-05-30 at 19 55 42

As for the .mgtAdvancedSettingsForm__button style, it is used in three places for the bottom bar button:

I don't see any visual change after removing this style, but I don't know where to find the data_source_management plugin on the website. @BSFishy could you please help me with it?

I hope I have fully covered this plugin, if you still have questions I will be happy to answer

@joshuarrrr
Copy link
Member

@SergeyMyssak See #4329 for design guidance for Advanced Settings which may avoid the need for some of the recommendations here.

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

5 participants