OpenSearch-Dashboards icon indicating copy to clipboard operation
OpenSearch-Dashboards copied to clipboard

[Cleanup] Reduce duplication in code for Initial Custom Branding

Open tmarkley opened this issue 3 years ago • 7 comments

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."

tmarkley avatar Oct 29 '21 22:10 tmarkley

Also the config should have a default value instead of /

kavilla avatar Nov 22 '21 06:11 kavilla

https://github.com/opensearch-project/OpenSearch-Dashboards/pull/941#discussion_r750720578

More granular error message

kavilla avatar Nov 22 '21 07:11 kavilla

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

kavilla avatar Mar 10 '22 18:03 kavilla

@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 avatar Apr 30 '22 02:04 ashwin-pc

@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 avatar May 03 '22 23:05 tmarkley

I also ran into a lot of this duplicated code as part of https://github.com/opensearch-project/OpenSearch-Dashboards/pull/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 avatar Jun 23 '22 23:06 joshuarrrr

TODO: break this issue down.

kavilla avatar Aug 01 '22 19:08 kavilla

@AMoo-Miki Can we mark this as complete via https://github.com/opensearch-project/OpenSearch-Dashboards/pull/4702, or were you still planning a follow-up PR?

joshuarrrr avatar Aug 28 '23 23:08 joshuarrrr