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.

Sign up for GitHub

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

Jump to bottom

[Cleanup] Reduce duplication in code for Initial Custom Branding #895

Closed
tmarkley opened this issue Oct 29, 2021 · 8 comments · Fixed by #4702
Closed

[Cleanup] Reduce duplication in code for Initial Custom Branding #895

tmarkley opened this issue Oct 29, 2021 · 8 comments · Fixed by #4702
Assignees
@AMoo-Miki
Labels
branding technical debt If not paid, jeapardizes long-term success and maintainability of the repository. v2.10.0

Comments

@tmarkley
Copy link
Contributor

tmarkley commented Oct 29, 2021 •
edited by kavilla
Loading

In PR #826 I suggested creating a more generic custom logo component to solve the common logic used across the application for custom branding. We don't have enough time to implement and test that refactoring work ahead of v1.2.0, so this issue serves as the follow-up task. We maintainers will address these changes ahead of the v1.3.0 release.

User story:
"As a developer or plugin author, I'd like to easily reference branding asset (logo or mark) without caring or knowing about the branding configuration, validation, or fallback logic."
kavilla added a commit to kavilla/OpenSearch-Dashboards-1 that referenced this issue Oct 30, 2021
@kavilla
[Branding] handle comments from PR
48e1c42 
Handling the helper function rename and grammar issues.

To avoid risk, we will not remove the duplicate code for 1.2 and
everything related to those comments (ie function renames).

That will be handled in 1.3. Here is the issue tracking it:

opensearch-project#895

Signed-off-by: Kawika Avilla <kavilla414@gmail.com>
@kavilla kavilla mentioned this issue Oct 30, 2021
Merged
kavilla added a commit that referenced this issue Oct 30, 2021
@kavilla
[Branding] handle comments from PR
e1deafd 
Handling the helper function rename and grammar issues.

To avoid risk, we will not remove the duplicate code for 1.2 and
everything related to those comments (ie function renames).

That will be handled in 1.3. Here is the issue tracking it:

#895

Signed-off-by: Kawika Avilla <kavilla414@gmail.com>
kavilla added a commit that referenced this issue Oct 30, 2021
@kavilla
[Branding] handle comments from PR
5edf030 
Handling the helper function rename and grammar issues.

To avoid risk, we will not remove the duplicate code for 1.2 and
everything related to those comments (ie function renames).

That will be handled in 1.3. Here is the issue tracking it:

#895

Signed-off-by: Kawika Avilla <kavilla414@gmail.com>
kavilla added a commit that referenced this issue Nov 1, 2021
@abbyhu2000 @kavilla
Custom Branding (#826)
9037b61 
* Make top left logo on the main screen configurable

Add a new config opensearchDashboards.branding.logoUrl in yaml file
for making top left corner logo on the main screen configurable. If
URL is invalid, the default OpenSearch logo will be shown.

Signed-off-by: Abby Hu <abigailhu2000@gmail.com>

* Welcome page title and logo configurable (#738)

Add two new configs branding.smallLogoUrl and branding.title
in the yaml file for making the welcome page logo and title
 configurable. If URL is invalid, the default branding will be shown.

Signed-off-by: Qingyang(Abby) Hu <abigailhu2000@gmail.com>

* Make loading page logo and title configurable (#746)

Add one new config branding.loadingLogoUrl for making loading page logo
configurable. URL can be in svg and gif format. If no loading logo is found,
the static logo with a horizontal bar loading bar will be shown. If logo is also
not found, the default OpenSearch loading logo and spinner will be shown.

Signed-off-by: Qingyang(Abby) Hu <abigailhu2000@gmail.com>

* Branding configs rename and improvement (#771)

Change config smallLogoUrl to logoUrl, config logoUrl to fullLogoUrl to emphasize that thumbnail version
of the logo will be used mostly in the application. Full version of the logo will only be used on the main
page nav bar. If full logo is not provided, thumbnail logo will be used on the nav bar. Some config improvement
includes fixing the validation error when inputting empty string, and add title validation function.

Signed-off-by: Qingyang(Abby) Hu <abigailhu2000@gmail.com>

* Branding config structure change and renaming (#793)

Change the branding related config to a map structure in the yml file.
Also rename the configs according to the official branding guidelines.
The full logo on the main page header will be called logo; the small
logo icon will be called mark.

Signed-off-by: Qingyang(Abby) Hu <abigailhu2000@gmail.com>

* Darkmode configurations for header logo, welcome logo and loading logo (#797)

Add dark mode configs in the yml file that allows user to configure a
dark mode version of the logo. When user toggles dark mode under the
Advanced Setting, the logo will be rendered accordingly.

Signed-off-by: Qingyang(Abby) Hu <abigailhu2000@gmail.com>

* Add favicon configuration (#801)

Added a configuration on favicon inside opensearchDashboards.branding
in the yml file. If user inputs a valid URL, we gurantee basic browser
favicon customization, while remaining places show the default browser/device
favicon icon. If user does not provide a valid URL for favicon, the
opensearch favicon icon will be used.

Signed-off-by: Qingyang(Abby) Hu <abigailhu2000@gmail.com>

* Make home page primary dashboard card logo and title configurable (#809)

Home page dashboard card logo and title can be customized by config
mark.defaultUrl and mark.darkModeUrl. Unit test and functional test
are also written.

Signed-off-by: Qingyang(Abby) Hu <abigailhu2000@gmail.com>

* Side menu logo configuration

Make logo for opensearch dashboard side menu be configurable.
Use config mark.defaultUrl and mark.darkModeUrl.

Signed-off-by: Abby Hu <abigailhu2000@gmail.com>

* Overview Header Logo Configuration

Make logo for opensearch dashboard overview header logo be configurable.
Use config mark.defaultUrl and mark.darkModeUrl.

Signed-off-by: Abby Hu <abigailhu2000@gmail.com>

* Redirect URL not allowed

Add an addtional parameter to the checkUrlValid function
so that max redirect count is 0. We do not allow URLs that
can be redirected because of potential security issues.

Signed-off-by: Abby Hu <abigailhu2000@gmail.com>

* Store default opensearch branding asset folder

Store the original opensearch branding logos in an asset folder,
instead of making API calls.

Signed-off-by: Abby Hu <abigailhu2000@gmail.com>

* [Branding] handle comments from PR

Handling the helper function rename and grammar issues.

To avoid risk, we will not remove the duplicate code for 1.2 and
everything related to those comments (ie function renames).

That will be handled in 1.3. Here is the issue tracking it:

#895

Signed-off-by: Kawika Avilla <kavilla414@gmail.com>

Co-authored-by: Kawika Avilla <kavilla414@gmail.com>
kavilla pushed a commit to kavilla/OpenSearch-Dashboards-1 that referenced this issue Nov 1, 2021
@abbyhu2000 @kavilla
[1.x] Custom Branding (opensearch-project#826)
f014d09 
* Make top left logo on the main screen configurable

Add a new config opensearchDashboards.branding.logoUrl in yaml file
for making top left corner logo on the main screen configurable. If
URL is invalid, the default OpenSearch logo will be shown.

Signed-off-by: Abby Hu <abigailhu2000@gmail.com>

* Welcome page title and logo configurable (opensearch-project#738)

Add two new configs branding.smallLogoUrl and branding.title
in the yaml file for making the welcome page logo and title
 configurable. If URL is invalid, the default branding will be shown.

Signed-off-by: Qingyang(Abby) Hu <abigailhu2000@gmail.com>

* Make loading page logo and title configurable (opensearch-project#746)

Add one new config branding.loadingLogoUrl for making loading page logo
configurable. URL can be in svg and gif format. If no loading logo is found,
the static logo with a horizontal bar loading bar will be shown. If logo is also
not found, the default OpenSearch loading logo and spinner will be shown.

Signed-off-by: Qingyang(Abby) Hu <abigailhu2000@gmail.com>

* Branding configs rename and improvement (opensearch-project#771)

Change config smallLogoUrl to logoUrl, config logoUrl to fullLogoUrl to emphasize that thumbnail version
of the logo will be used mostly in the application. Full version of the logo will only be used on the main
page nav bar. If full logo is not provided, thumbnail logo will be used on the nav bar. Some config improvement
includes fixing the validation error when inputting empty string, and add title validation function.

Signed-off-by: Qingyang(Abby) Hu <abigailhu2000@gmail.com>

* Branding config structure change and renaming (opensearch-project#793)

Change the branding related config to a map structure in the yml file.
Also rename the configs according to the official branding guidelines.
The full logo on the main page header will be called logo; the small
logo icon will be called mark.

Signed-off-by: Qingyang(Abby) Hu <abigailhu2000@gmail.com>

* Darkmode configurations for header logo, welcome logo and loading logo (opensearch-project#797)

Add dark mode configs in the yml file that allows user to configure a
dark mode version of the logo. When user toggles dark mode under the
Advanced Setting, the logo will be rendered accordingly.

Signed-off-by: Qingyang(Abby) Hu <abigailhu2000@gmail.com>

* Add favicon configuration (opensearch-project#801)

Added a configuration on favicon inside opensearchDashboards.branding
in the yml file. If user inputs a valid URL, we gurantee basic browser
favicon customization, while remaining places show the default browser/device
favicon icon. If user does not provide a valid URL for favicon, the
opensearch favicon icon will be used.

Signed-off-by: Qingyang(Abby) Hu <abigailhu2000@gmail.com>

* Make home page primary dashboard card logo and title configurable (opensearch-project#809)

Home page dashboard card logo and title can be customized by config
mark.defaultUrl and mark.darkModeUrl. Unit test and functional test
are also written.

Signed-off-by: Qingyang(Abby) Hu <abigailhu2000@gmail.com>

* Side menu logo configuration

Make logo for opensearch dashboard side menu be configurable.
Use config mark.defaultUrl and mark.darkModeUrl.

Signed-off-by: Abby Hu <abigailhu2000@gmail.com>

* Overview Header Logo Configuration

Make logo for opensearch dashboard overview header logo be configurable.
Use config mark.defaultUrl and mark.darkModeUrl.

Signed-off-by: Abby Hu <abigailhu2000@gmail.com>

* Redirect URL not allowed

Add an addtional parameter to the checkUrlValid function
so that max redirect count is 0. We do not allow URLs that
can be redirected because of potential security issues.

Signed-off-by: Abby Hu <abigailhu2000@gmail.com>

* Store default opensearch branding asset folder

Store the original opensearch branding logos in an asset folder,
instead of making API calls.

Signed-off-by: Abby Hu <abigailhu2000@gmail.com>

* [Branding] handle comments from PR

Handling the helper function rename and grammar issues.

To avoid risk, we will not remove the duplicate code for 1.2 and
everything related to those comments (ie function renames).

That will be handled in 1.3. Here is the issue tracking it:

opensearch-project#895

Signed-off-by: Kawika Avilla <kavilla414@gmail.com>

Co-authored-by: Kawika Avilla <kavilla414@gmail.com>

Backport PR:
opensearch-project#897
@kavilla kavilla mentioned this issue Nov 1, 2021
Merged
kavilla added a commit that referenced this issue Nov 1, 2021
@kavilla @abbyhu2000
[1.x] Custom Branding (#826) (#898)
1c28cfd 
* Make top left logo on the main screen configurable

Add a new config opensearchDashboards.branding.logoUrl in yaml file
for making top left corner logo on the main screen configurable. If
URL is invalid, the default OpenSearch logo will be shown.

Signed-off-by: Abby Hu <abigailhu2000@gmail.com>

* Welcome page title and logo configurable (#738)

Add two new configs branding.smallLogoUrl and branding.title
in the yaml file for making the welcome page logo and title
 configurable. If URL is invalid, the default branding will be shown.

Signed-off-by: Qingyang(Abby) Hu <abigailhu2000@gmail.com>

* Make loading page logo and title configurable (#746)

Add one new config branding.loadingLogoUrl for making loading page logo
configurable. URL can be in svg and gif format. If no loading logo is found,
the static logo with a horizontal bar loading bar will be shown. If logo is also
not found, the default OpenSearch loading logo and spinner will be shown.

Signed-off-by: Qingyang(Abby) Hu <abigailhu2000@gmail.com>

* Branding configs rename and improvement (#771)

Change config smallLogoUrl to logoUrl, config logoUrl to fullLogoUrl to emphasize that thumbnail version
of the logo will be used mostly in the application. Full version of the logo will only be used on the main
page nav bar. If full logo is not provided, thumbnail logo will be used on the nav bar. Some config improvement
includes fixing the validation error when inputting empty string, and add title validation function.

Signed-off-by: Qingyang(Abby) Hu <abigailhu2000@gmail.com>

* Branding config structure change and renaming (#793)

Change the branding related config to a map structure in the yml file.
Also rename the configs according to the official branding guidelines.
The full logo on the main page header will be called logo; the small
logo icon will be called mark.

Signed-off-by: Qingyang(Abby) Hu <abigailhu2000@gmail.com>

* Darkmode configurations for header logo, welcome logo and loading logo (#797)

Add dark mode configs in the yml file that allows user to configure a
dark mode version of the logo. When user toggles dark mode under the
Advanced Setting, the logo will be rendered accordingly.

Signed-off-by: Qingyang(Abby) Hu <abigailhu2000@gmail.com>

* Add favicon configuration (#801)

Added a configuration on favicon inside opensearchDashboards.branding
in the yml file. If user inputs a valid URL, we gurantee basic browser
favicon customization, while remaining places show the default browser/device
favicon icon. If user does not provide a valid URL for favicon, the
opensearch favicon icon will be used.

Signed-off-by: Qingyang(Abby) Hu <abigailhu2000@gmail.com>

* Make home page primary dashboard card logo and title configurable (#809)

Home page dashboard card logo and title can be customized by config
mark.defaultUrl and mark.darkModeUrl. Unit test and functional test
are also written.

Signed-off-by: Qingyang(Abby) Hu <abigailhu2000@gmail.com>

* Side menu logo configuration

Make logo for opensearch dashboard side menu be configurable.
Use config mark.defaultUrl and mark.darkModeUrl.

Signed-off-by: Abby Hu <abigailhu2000@gmail.com>

* Overview Header Logo Configuration

Make logo for opensearch dashboard overview header logo be configurable.
Use config mark.defaultUrl and mark.darkModeUrl.

Signed-off-by: Abby Hu <abigailhu2000@gmail.com>

* Redirect URL not allowed

Add an addtional parameter to the checkUrlValid function
so that max redirect count is 0. We do not allow URLs that
can be redirected because of potential security issues.

Signed-off-by: Abby Hu <abigailhu2000@gmail.com>

* Store default opensearch branding asset folder

Store the original opensearch branding logos in an asset folder,
instead of making API calls.

Signed-off-by: Abby Hu <abigailhu2000@gmail.com>

* [Branding] handle comments from PR

Handling the helper function rename and grammar issues.

To avoid risk, we will not remove the duplicate code for 1.2 and
everything related to those comments (ie function renames).

That will be handled in 1.3. Here is the issue tracking it:

#895

Signed-off-by: Kawika Avilla <kavilla414@gmail.com>

Co-authored-by: Kawika Avilla <kavilla414@gmail.com>

Backport PR:
#897

Co-authored-by: Qingyang(Abby) Hu <abigailhu2000@gmail.com>
seanneumann pushed a commit that referenced this issue Nov 2, 2021
@kavilla @abbyhu2000
[1.x] Custom Branding (#826) (#898)
64441cb 
* Make top left logo on the main screen configurable

Add a new config opensearchDashboards.branding.logoUrl in yaml file
for making top left corner logo on the main screen configurable. If
URL is invalid, the default OpenSearch logo will be shown.

Signed-off-by: Abby Hu <abigailhu2000@gmail.com>

* Welcome page title and logo configurable (#738)

Add two new configs branding.smallLogoUrl and branding.title
in the yaml file for making the welcome page logo and title
 configurable. If URL is invalid, the default branding will be shown.

Signed-off-by: Qingyang(Abby) Hu <abigailhu2000@gmail.com>

* Make loading page logo and title configurable (#746)

Add one new config branding.loadingLogoUrl for making loading page logo
configurable. URL can be in svg and gif format. If no loading logo is found,
the static logo with a horizontal bar loading bar will be shown. If logo is also
not found, the default OpenSearch loading logo and spinner will be shown.

Signed-off-by: Qingyang(Abby) Hu <abigailhu2000@gmail.com>

* Branding configs rename and improvement (#771)

Change config smallLogoUrl to logoUrl, config logoUrl to fullLogoUrl to emphasize that thumbnail version
of the logo will be used mostly in the application. Full version of the logo will only be used on the main
page nav bar. If full logo is not provided, thumbnail logo will be used on the nav bar. Some config improvement
includes fixing the validation error when inputting empty string, and add title validation function.

Signed-off-by: Qingyang(Abby) Hu <abigailhu2000@gmail.com>

* Branding config structure change and renaming (#793)

Change the branding related config to a map structure in the yml file.
Also rename the configs according to the official branding guidelines.
The full logo on the main page header will be called logo; the small
logo icon will be called mark.

Signed-off-by: Qingyang(Abby) Hu <abigailhu2000@gmail.com>

* Darkmode configurations for header logo, welcome logo and loading logo (#797)

Add dark mode configs in the yml file that allows user to configure a
dark mode version of the logo. When user toggles dark mode under the
Advanced Setting, the logo will be rendered accordingly.

Signed-off-by: Qingyang(Abby) Hu <abigailhu2000@gmail.com>

* Add favicon configuration (#801)

Added a configuration on favicon inside opensearchDashboards.branding
in the yml file. If user inputs a valid URL, we gurantee basic browser
favicon customization, while remaining places show the default browser/device
favicon icon. If user does not provide a valid URL for favicon, the
opensearch favicon icon will be used.

Signed-off-by: Qingyang(Abby) Hu <abigailhu2000@gmail.com>

* Make home page primary dashboard card logo and title configurable (#809)

Home page dashboard card logo and title can be customized by config
mark.defaultUrl and mark.darkModeUrl. Unit test and functional test
are also written.

Signed-off-by: Qingyang(Abby) Hu <abigailhu2000@gmail.com>

* Side menu logo configuration

Make logo for opensearch dashboard side menu be configurable.
Use config mark.defaultUrl and mark.darkModeUrl.

Signed-off-by: Abby Hu <abigailhu2000@gmail.com>

* Overview Header Logo Configuration

Make logo for opensearch dashboard overview header logo be configurable.
Use config mark.defaultUrl and mark.darkModeUrl.

Signed-off-by: Abby Hu <abigailhu2000@gmail.com>

* Redirect URL not allowed

Add an addtional parameter to the checkUrlValid function
so that max redirect count is 0. We do not allow URLs that
can be redirected because of potential security issues.

Signed-off-by: Abby Hu <abigailhu2000@gmail.com>

* Store default opensearch branding asset folder

Store the original opensearch branding logos in an asset folder,
instead of making API calls.

Signed-off-by: Abby Hu <abigailhu2000@gmail.com>

* [Branding] handle comments from PR

Handling the helper function rename and grammar issues.

To avoid risk, we will not remove the duplicate code for 1.2 and
everything related to those comments (ie function renames).

That will be handled in 1.3. Here is the issue tracking it:

#895

Signed-off-by: Kawika Avilla <kavilla414@gmail.com>

Co-authored-by: Kawika Avilla <kavilla414@gmail.com>

Backport PR:
#897

Co-authored-by: Qingyang(Abby) Hu <abigailhu2000@gmail.com>
@kavilla
Copy link
Member

kavilla commented Nov 22, 2021

Also the config should have a default value instead of /

@kavilla
Copy link
Member

kavilla commented Nov 22, 2021

#941 (comment)

More granular error message

@kavilla kavilla mentioned this issue Nov 22, 2021
Merged
4 tasks
@kavilla kavilla self-assigned this Jan 5, 2022
@kavilla kavilla added v1.4.0 and removed v1.3.0 labels Mar 10, 2022
@kavilla
Copy link
Member

kavilla commented Mar 10, 2022

Moving to 1.4.0 since this work was not able to be completed by 1.3

@ashwin-pc
Copy link
Member

@tmarkley @kavilla #1448 follows the pattern set by the original branding PR and introduces even more duplicate code. Do you think its a good opportunity to start some of the deduplication in that PR or should we follow the same pattern there and consider the deduplication a separate task by itself?

#1448 introduces quite a few properties that almost doubles the code in related branding files because of the original pattern.

@ashwin-pc ashwin-pc mentioned this issue May 3, 2022
Closed
7 tasks
@tmarkley
Copy link
Contributor Author

tmarkley commented May 3, 2022

@tmarkley @kavilla #1448 follows the pattern set by the original branding PR and introduces even more duplicate code. Do you think its a good opportunity to start some of the deduplication in that PR or should we follow the same pattern there and consider the deduplication a separate task by itself?

#1448 introduces quite a few properties that almost doubles the code in related branding files because of the original pattern.

@ashwin-pc thanks for bringing that up! I think that it is worthwhile to address the deduplication and cleanup of that code ahead of merging these new changes. @kavilla do we need to re-assign this?

@tmarkley tmarkley added the technical debt If not paid, jeapardizes long-term success and maintainability of the repository. label May 25, 2022
@joshuarrrr joshuarrrr added v2.2.0 and removed v2.1.0 labels Jun 23, 2022
@joshuarrrr
Copy link
Member

I also ran into a lot of this duplicated code as part of #1586, and was able to simplify some things in place, but as @tmarkley's review comments indicate, much of the duplication is due to the fact that we simply pass along all the raw configuration values as part of the metadata, and require all consuming components to implement their own logic.

Instead, the branding service should handle much of the conditional rendering logic (dark mode, fallbacks, etc.), and only vend out hooks or components to retrieve those (mark, logo, application title).

@joshuarrrr joshuarrrr mentioned this issue Jun 23, 2022
Merged
7 tasks
@joshuarrrr joshuarrrr removed the v2.2.0 label Jul 8, 2022
@kavilla
Copy link
Member

kavilla commented Aug 1, 2022

TODO: break this issue down.

@kavilla kavilla mentioned this issue Aug 2, 2022
Open
@AMoo-Miki AMoo-Miki assigned AMoo-Miki and unassigned kavilla and abbyhu2000 Aug 7, 2023
@joshuarrrr
Copy link
Member

@AMoo-Miki Can we mark this as complete via #4702, or were you still planning a follow-up PR?

@AMoo-Miki AMoo-Miki mentioned this issue Sep 1, 2023
Merged
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@AMoo-Miki AMoo-Miki

Labels
branding technical debt If not paid, jeapardizes long-term success and maintainability of the repository. v2.10.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Refactor logo usage AMoo-Miki/OpenSearch-Dashboards
6 participants
@joshuarrrr @AMoo-Miki @tmarkley @kavilla @ashwin-pc @abbyhu2000