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

Enable override of sidebar selected indicator color when using createTheme #1880

Merged

Conversation

theplatformer
Copy link
Contributor

Hey, I just made a Pull Request!

I am working on an internal POC of Backstage and applying some simple brand colors with a custom theme but found the sidebar selected item color is not included in PaletteAdditions and is hardcoded in the sidebar layout, so cannot be set via createTheme.

With this the default themes are unchanged but are now using refactored sidebar palette properties...

sidebar: {
    backgroundColor: string;
    selectedIndicatorColor: string;
};

Default-Dark
Default-Light

Using a custom theme now allows the selected indicator to be overridden as desired...

sidebar: {
    backgroundColor: '#424242',
    selectedIndicatorColor: '#FF3209',
},

Custom-Dark
Custom-Light

I hope my understanding is correct. I am brand new to React and am working to understand the Backstage codebase.
Thanks for the awesome project, this is my first every pull request!

✔️ Checklist

  • All tests are passing yarn test
  • Screenshots attached (for UI changes)
  • Relevant documentation updated
  • Prettier run on changed files
  • Tests added for new functionality
  • Regression tests added for bug fixes

@theplatformer theplatformer requested a review from a team as a code owner August 9, 2020 11:32
Copy link
Member

@Rugvip Rugvip left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Implementation looks good, just a bunch of thoughts on naming 😅

packages/theme/src/types.ts Outdated Show resolved Hide resolved
packages/theme/src/types.ts Outdated Show resolved Hide resolved
@@ -43,7 +43,10 @@ type PaletteAdditions = {
linkHover: string;
link: string;
gold: string;
sidebar: string;
sidebar: {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're up for extending this change a bit I think we should rename this, since sidebar is a bit of an implementation detail. Something like topNav or navigation would make sense I think, what do others think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, fair enough. I think I'd go with navigation just so consistent with Material-UI (have pushed changes to that effect) but more than happy to take direction here if there is a preference.

@stefanalund
Copy link
Contributor

Thank you @ContrarianChris and welcome to the Backstage community! 🎉

@stefanalund stefanalund merged commit 1c559bc into backstage:master Aug 11, 2020
@theplatformer theplatformer deleted the sidebar-indicator-color-override branch August 11, 2020 09:04
benjdlambert added a commit that referenced this pull request Aug 12, 2020
…g-in-gql

* 'master' of github.com:spotify/backstage: (34 commits)
  fix(roadmap): switch links to milestones for v2 and v3
  fix(roadmap): adjust project roadmap and link to use cases for v2
  chore(deps-dev): bump @types/jest from 26.0.7 to 26.0.9 (#1905)
  Added v2 to techdocs readme
  Update README with new copy (#1900)
  Exclude microsite from the frontend CI workflow
  workflows: run broken link check in frontend CI
  docs: fix broken links
  docs: add basic script to verify local documentation links
  lint ignore microsite
  Add microsite to main repo
  Update design.md (#1898)
  Fix create-an-app.md output image (#1897)
  Enable override of sidebar selected indicator color when using createTheme (#1880)
  Remove extraneous tab
  test(scaffolder/docker): added some tests to check the home cariable
  chore(scaffolder): fixing some of the docs and making things a little cleanern
  chore(scaffolder): set a home directory for cookiecutter now we have new username and group id
  chores(scaffolder): trying something new
  chore(scaffolder): set the working directory default as the temp foler
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants