Open SergeyMyssak opened 1 year ago
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?
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.
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:
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.
https://github.com/opensearch-project/OpenSearch-Dashboards/issues/4182
@KrooshalUX, I have a question about the incremental
parameter. Should we remove it? Everything seems to work fine without it.
2) 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.
3) 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.
4) 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:
Field
component in the OUI or a generic component in Dashboards, and then use it in advanced_settings
and saved_objects_management
plugins. 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.
By making these functionalities, we may encounter problems number 1, 2, 3, and 4.
5) 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.
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
@SergeyMyssak See https://github.com/opensearch-project/OpenSearch-Dashboards/issues/4329 for design guidance for Advanced Settings which may avoid the need for some of the recommendations here.
Audit
I performed the OUI Usage audit to the
advanced_settings
plugin and got the following results.This plugin has 8 custom components:
management_app/components
: advanced_settings_voice_announcement, call_outs, field, form, searchcomponent_registry
: PageTitle, PageSubtitle, PageFooterThe image below shows all the components listed except
advanced_settings_voice_announcement
,PageSubtitle
andPageFooter
, 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
PageSubtitle
andPageFooter
are functions that returnnull
and can be removed if they are not to be used in the futurescss
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 https://github.com/opensearch-project/OpenSearch-Dashboards/blob/a8ace28a97f5b178e5e767db03dc8d38046ce14e/src/plugins/advanced_settings/public/management_app/_advanced_settings.scss#L76-L78