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

ui: add optional white background to PageConfig #87189

Conversation

absterr08
Copy link
Contributor

Previously, the PageConfig component had a fixed grey
background to match the page background, since it is a sticky
component that needs a background color to cover content that
scrolls beneath it. However, the background color is changing
to white in CockroachCloud, so the background color needs to
be able to match the new background. Until we apply the layout
changes to db-console, this change allows us to specify a
different background color in CockroachCloud.

This change adds an optional hasWhiteBackground prop to the
PageConfig component, which sets the background color
accordingly. This props is also added to the StatementsPage and
TransactionsPage components, and a pageHasWhiteBackground
bool is added to UIConfig, so that it can be set when
configuring the components in CockroachCloud.

Release note: None

Without white background
Screen Shot 2022-08-31 at 12 11 39 PM

With white background
Screen Shot 2022-08-31 at 12 11 56 PM

Previously, the PageConfig component had a fixed grey
background to match the page background, since it is a sticky
component that needs a background color to cover content that
scrolls beneath it. However, the background color is changing
to white in CockroachCloud, so the background color needs to
be able to match the new background. Until we apply the layout
changes to `db-console`, this change allows us to specify a
different background color in CockroachCloud.

This change adds an optional `hasWhiteBackground` prop to the
`PageConfig` component, which sets the background color
accordingly. This props is also added to the `StatementsPage` and
`TransactionsPage` components, and a `pageHasWhiteBackground`
bool is added to `UIConfig`, so that it can be set when
configuring the components in CockroachCloud.

Release note: None
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@absterr08 absterr08 requested a review from a team August 31, 2022 16:27
@absterr08
Copy link
Contributor Author

Hello! 👋 This is my first cluster-ui PR so I'm ready for a roast :)
A few questions I had:
I'd like to apply these changes to versions 21.2 and beyond, since 21.1 is no longer officially supported. What is the correct way to apply these to 21.2 and beyond? Should I just open separate PRs, in addition to this one, in release-21.2 and in master?
And as for updating the package version, should I include the version change in this PR or a subsequent one?

Thanks in advance!

@absterr08 absterr08 requested review from maryliag and xinhaoz August 31, 2022 18:16
Copy link
Contributor

@maryliag maryliag left a comment

Choose a reason for hiding this comment

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

Answering your questions first:
I would recommend creating this PR on master and then backporting to 22.1 and 21.2 branches. It's not common to create something directly on a specific branch if you want to be also on the latest version.
After you have merged, you would need to do go each branch (master, 22.1 and 21.2) and create a new cluster-ui from it. Only after you publish the version is that you create the PR that updates the version on the branch to match the one on npm.
I made a recording about how to publish on cluster-ui if that helps: https://drive.google.com/file/d/1kDdZmnjHjYH0s0VIPtvv7RjlnbPKLEsm/view

About the PR itself:
My concern with this is how specific your prop is, because if you decide to change the color, you would need to update the prop name. Or if there are other things you want to change, you would need to create a prop for each thing.
We do have a flag to know when the page is being loaded from DB vs CC, so you can use that instead. This way is not very specific and you can just decide the class to add based on that flag.
Here is an example of using the flag:

const isCockroachCloud = useContext(CockroachCloudContext);

The only problem is that this flag only exists on master, so this would need to be backported first
This was the PR (https://github.com/cockroachdb/cockroach/pull/86264/files#diff-30a225059158cf9b9a1b84587379d897f135acbbacd4c49d9c4e6828f966a8a3R64) that introduced the flag, but this PR has more than just that, so we would need a backport with just that part. Maybe @xinhaoz can help you here by creating a backport with just the flag

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @xinhaoz)

@xinhaoz
Copy link
Member

xinhaoz commented Sep 1, 2022

Hello! Yes I can definitely open up a backport with just the context changes.

@absterr08
Copy link
Contributor Author

Ah, the isCockroachCloud flag would be much better for this. I'll go ahead and open this PR on master then. Thank you both!

@absterr08
Copy link
Contributor Author

Closing in favor of #87313

@absterr08 absterr08 closed this Sep 1, 2022
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.

4 participants