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

[Branding] implement to reduce validation logic #2044

Open
kavilla opened this issue Aug 2, 2022 · 8 comments
Open

[Branding] implement to reduce validation logic #2044

kavilla opened this issue Aug 2, 2022 · 8 comments
Assignees
Labels
branding enhancement New feature or request

Comments

@kavilla
Copy link
Member

kavilla commented Aug 2, 2022

Is your feature request related to a problem? Please describe.

Origin: #2045

Describe the solution you'd like

To be added, once #2045. Once the parent issue is completed then we can break discuss the solution

Requirements

  • Propose and break down implementation details.
@abbyhu2000
Copy link
Member

abbyhu2000 commented Aug 4, 2022

Here are some initial design spike result for the first two possible implementation, and still researching on the third one(branding plugin):

  1. Extend rendering service: [Draft] Extend rendering service  abbyhu2000/OpenSearch-Dashboards#5
    The con of this solution is straightforward with low risk and have minimal impact. However, if we expand or add more branding logics later on, this solution might not be the best because rendering service can get too crowded with all the branding logics while the main purpose for rendering service should be rendering metadata.

  2. Create a new branding service: [Draft] Implementing a new branding service abbyhu2000/OpenSearch-Dashboards#4
    For this solution, a new service has been added to the core system. From the current codebase, it seems like the core system only contains the most essential services to the application, so I am not sure if it is the best solution to add a branding service in the core just to handle custom branding.

@kavilla @joshuarrrr @ashwin-pc

@abbyhu2000
Copy link
Member

abbyhu2000 commented Aug 8, 2022

  1. Draft PR for branding plugin (still work in progress): Branding plugin abbyhu2000/OpenSearch-Dashboards#6
    In the draft PR, created a branding plugin that read the branding configs from the opensearch_dashboards.yml file. Currently the logos and applications we wish to customize are distributed in core/server, core/public and other plugins.

We can create service(for example, like a branding assignment service that handles all the validation logic and duplicated logic for darkmode etc.) in the public/ folder so it can be exposed and other plugins and the core can call the service when they need custom branding. It is a viable solution since i found previous examples of plugins that communicate with core/server, core/public and other plugins. The con of this approach is that it will take significant more time to build a plugin and it seems like the current relatively simple custom branding functionality does not need a separate plugin to support it. But in the future, if we want to make more branding and theming configuration, then it would be nice to have a separate plugin dedicated for this purpose.

@ashwin-pc
Copy link
Member

Since our goal here is to reduce logic, I still think that my suggestion here would simplify branding significantly.

@abbyhu2000
Copy link
Member

abbyhu2000 commented Aug 11, 2022

  1. Use i18n external plugin for custom title configuration: Use i18n plugin for custom title configuration abbyhu2000/OpenSearch-Dashboards#7

Users can install the external plugin and when they upload the json translation files, they can customize the application title at the same time. We can have a doc to document or mark all the application title ids so it is easy for users to change. Currently, not all references of application title is registered with i18n. I added a couple more ids for example, such as the overview title, side menu title and the home page title and test them out with two locales. There are more title references throughout the application.

We currently only checking if title is less than 36 characters for title validation, need to think of a different way of title validation if using the i18n framework.

@abbyhu2000
Copy link
Member

  1. Use asset folder to store default branding logos and user can directly replace those default images if they choose to.

Our current implementation already stores all the default opensearch logos' and marks' svg images in an asset folder (src/core/server/core_app/assets/default_branding). We can move this folder to a root level folder and user can have a second option to configure branding on top of using the yml file.

@joshuarrrr
Copy link
Member

Our current implementation already stores all the default opensearch logos' and marks' svg images in an asset folder (src/core/server/core_app/assets/default_branding). We can move this folder to a root level folder and user can have a second option to configure branding on top of using the yml file.

Rather than moving them,we might consider copying them to a root level folder instead. That would allow the user to have a safe space to make changes or overwrite the files, with the option to restore or reset if necessary.

@ashwin-pc
Copy link
Member

@joshuarrrr Oooh, I like this more

@abbyhu2000
Copy link
Member

After our discussion, we decided to use solution 1 (extend rendering service) to reduce the duplicated branding logics for now. In the future, we will shift the branding configuration to solution 4 & 5 using asset folder and i18n plugin with the consideration of community feedback. More details to follow.

@joshuarrrr joshuarrrr mentioned this issue Sep 19, 2022
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branding enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants