OpenSearch-Dashboards
OpenSearch-Dashboards copied to clipboard
[Cleanup] Reduce duplication in code for Initial Custom Branding
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."
Also the config should have a default value instead of /
https://github.com/opensearch-project/OpenSearch-Dashboards/pull/941#discussion_r750720578
More granular error message
Moving to 1.4.0 since this work was not able to be completed by 1.3
@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.
@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?
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).
TODO: break this issue down.
@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?