-
Notifications
You must be signed in to change notification settings - Fork 338
Display version information to web #3328
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
Changes from 4 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -206,11 +206,23 @@ export const Header: FC = memo(function Header() { | |||||
| Events | ||||||
| </Link> | ||||||
| </MenuItem> | ||||||
| <MenuItem> | ||||||
| <MenuItem divider={true}> | ||||||
| <Link component={RouterLink} to={PAGE_PATH_SETTINGS}> | ||||||
| Settings | ||||||
| </Link> | ||||||
| </MenuItem> | ||||||
| <MenuItem> | ||||||
| <Link | ||||||
| href="https://pipecd.dev/docs/" | ||||||
| target="_blank" | ||||||
| rel="noreferrer" | ||||||
| > | ||||||
| Docs | ||||||
| </Link> | ||||||
| </MenuItem> | ||||||
| <MenuItem disabled={true} dense={true}> | ||||||
| {process.env.STABLE_VERSION?.substring(0, 10)} | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
nits
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. btw, not sure why do we need to cut the string, why not just show the full version string? Since the last part is the commit hash and can be used to refer to source code on pipecd repo, which could be useful in some cases 👀
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I get your point 👍 Then, how about remove the
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That is exactly what I was thinking about.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. About the version string, I think showing the full one is fine. Because on the production it is not that long.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks so beautiful! Just a nit: How do you think about moving the menu popup to be lower to avoid overlapping the tab bar?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Than sounds good to me. |
||||||
| </MenuItem> | ||||||
| </Menu> | ||||||
| </AppBar> | ||||||
| ); | ||||||
|
|
||||||



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.