-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Added logo #5150
Added logo #5150
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:)
@@ -104,6 +104,7 @@ const Navigation: FC<PathPrefixProps & NavigationProps> = ({ pathPrefix, thanosC | |||
<Navbar className="mb-3" dark color="dark" expand="md" fixed="top"> | |||
<NavbarToggler onClick={toggle} className="mr-2" /> | |||
<Link className="navbar-brand" to={`${pathPrefix}${defaultRoute}`}> | |||
<img src="https://thanos.io/icon-dark.png" alt="thanos" style={{ width: '2rem', marginRight: '0.5rem' }} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe it would be interesting to have it locally. Otherwise server that doesn't have access to internet won't be able to display the logo :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In case it can help, this is how we did it in Prometheus : prometheus/prometheus#10236
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @Nexucis looks alright?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you run the command make assets
at the root of the project ? And then commit the modification of the files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I already did that, there isn't any modification apart from what I committed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
after running make assets
:
git status
On branch logo
Your branch is up to date with 'origin/logo'.
Changes not staged for commit:
(use "git add <file>..." to update what will be committed)
(use "git restore <file>..." to discard changes in working directory)
modified: pkg/ui/bindata.go
no changes added to commit (use "git add" and/or "git commit -a")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe you didn't push every changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
excepting the asset issue, it looks good to me. I'm just wondering if it would be cleaner to put the style in a dedicated css class.
@onprem any thought around this ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe you didn't push every changes
Do you want me to add the bindata.go file as well, it had a lot of changes that's why I didn't add it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes that's exactly what you have to do
Love it! |
Soooo after talking a bit with @matej-g, it appears that the issue regarding the doc is tracked with #4732 and there is a short fix with #5167. So I would suggest that you revert the changes related to the doc @tend2infinity in this PR. Hopefully once #5167 is merged, the CI in this PR will be green. |
you still have to run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
cool, now we are good @tend2infinity, thanks ! back to you guys @kakkoyun @metalmatze :) |
ah no wait, you have some conflict @tend2infinity. Probably better to rebase the PR with the last changes from the main branch and then re-run |
Yup, I rebased, I feel now we should be good to go |
It looks like the file |
Seems like docs are failing for almost all of my PRs 👀 |
@tend2infinity could you rebase on main? Docs are building correctly there and there were done changes recently |
Friendly ping (: |
@tend2infinity if you are not available anymore, I could take back your PR and rebase it if you want, so we can merge it :). |
Hey @Nexucis really sorry couldn't catch up, actually have my exams right now, will update this as soon as I get time |
@tend2infinity Can I take this up? 👀 |
any chance you have some time to rebase this PR @tend2infinity ? |
Its been a while since I looked upon this issue and right now um facing a lot of conflicts while rebasing the branch, maybe we can close this issue and open a clone of this and make the same changes in a new branch |
if it's too hard to rebase, maybe you could simply merge the main branch in yours. And if the conflicts are still too hard to resolve, another way around to do it, is to erase your local branch with the last commit from the main branch. And then copy and past the changes you can find in this PR. Commit your changes, and force push. So it doesn't change the branch and so the PR. |
Signed-off-by: tend2infinity <somu12.ss@gmail.com>
Done exactly the same, I guess it must be fixed now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @tend2infinity.
It looks good to me :).
if someone from the thanos team can rerun the github action, it will hopefully work :). And then probably it will be the good time to merge if you are agree too ;) |
Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward? This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
yes it is still relevant, but I have no idea why it has not been merged. Perhaps there was still a blocking point |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tend2infinity could you regenerate the bindata file? After that we can merge 🚢
Hello 👋 Looks like there was no activity on this amazing PR for the last 30 days. |
Hi @tend2infinity, are you still planning to finish this pr? If not, I am happy to help resolve the merge conflict and get this in! |
Hello 👋 Looks like there was no activity on this amazing PR for the last 30 days. |
this PR can be closed as the logo has been added thanks to the PR #6264 :) |
Signed-off-by: soumya somu12.ss@gmail.com
Issue #5121
Changes
Added logo
Verification
Tested locally
UI Screenshot