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

Fixes #36872 - Banner for different foreman instances #9872

Merged
merged 1 commit into from
Jan 12, 2024

Conversation

MariaAga
Copy link
Member

@MariaAga MariaAga commented Oct 23, 2023

example: image
settings: image

@github-actions github-actions bot added the UI label Oct 23, 2023
@MariaAga MariaAga changed the title draft - Banner for different foreman instances Fixes #36872 - Banner for different foreman instances Oct 30, 2023
@MariaAga MariaAga marked this pull request as ready for review October 30, 2023 12:51
@@ -81,9 +81,14 @@
collection: timezones)
setting('instance_title',
type: :string,
description: N_("The instance title is shown on the top navigation bar (requires a page reload)."),
description: N_("The instance title is shown under the header in a banner (requires a page reload)."),
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a detail, but isn't it "above" rather than "under" the header?
Other than that, everything looks great, I also tried multiple banner colors

Copy link
Member Author

Choose a reason for hiding this comment

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

Youre right, thanks!

@MariaAga MariaAga force-pushed the instance-banner branch 2 times, most recently from dcb68b1 to cb7be65 Compare November 20, 2023 14:57
@adamruzicka
Copy link
Contributor

Needs a rebase now

@MariaAga
Copy link
Member Author

MariaAga commented Jan 8, 2024

Thanks, rebased, (edited the layout test file)

Copy link
Member

@ofedoren ofedoren left a comment

Choose a reason for hiding this comment

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

Thanks, @MariaAga. One question though: should we validate color value via settings validator or maybe have somewhere mentioning that invalid values will be ignored and the default (#000000) value will be used instead?

I mean, even if it's obvious, some users might think that the feature doesn't work because of e.g. a simple typo.

@MariaAga
Copy link
Member Author

@ofedoren Thanks, I rephrased the setting to mention that.

Copy link
Member

@ofedoren ofedoren left a comment

Choose a reason for hiding this comment

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

Thanks, @MariaAga, I guess that could work :)

@ofedoren ofedoren merged commit 8b0b897 into theforeman:develop Jan 12, 2024
22 of 23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants