Improving main layout #3367
Improving main layout #3367
Conversation
williamjallen
left a comment
williamjallen
left a comment
There was a problem hiding this comment.
Thanks for this contribution. A few initial comments:
- This PR contains a bunch of unrelated changes. Please create separate PRs for each change to keep code review, release note tags, and documentation separate.
- Please add before/after screenshots of all UI changes to the PR description.
- Changes to table styles should be applied to all tables across the site. Making every table consistent is something I've worked hard to achieve over the last few years.
- There are quite a few comments which appear to be artifacts of LLM usage. Please review the changes and make sure the comments are helpful to developers long-term.
| "email": "[email protected]", | ||
| "issues": "https://github.com/Kitware/CDash/issues", | ||
| "source": "https://github.com/Kitware/CDash", | ||
| "wiki": "http://public.kitware.com/Wiki/CDash" |
There was a problem hiding this comment.
Please submit this as a separate pull request for documentation/release note purposes. Also, trailing commas are prohibited in JSON, which causes composer install to fail.
There was a problem hiding this comment.
The changes to this file should be split into a separate PR for documentation and release note purposes. It's worth noting that I'm actively working on a complete rewrite of this page which I hope to include in CDash 3.9.
There was a problem hiding this comment.
While I agree that these controls should live somewhere else, I don't think the header is the right place to put them. I'm trying to slowly remove page-specific parts of the header so it's generic across all pages.
| $response['menu']['current'] = "$base_url"; | ||
|
|
||
| // Only show 'current' button if we're not already on the current date | ||
| // Check if a date was explicitly provided in the URL | ||
| $date_specified = isset($_GET['date']); | ||
|
|
||
| if ($date_specified && $response['date'] !== $response['currentdate']) { | ||
| $response['menu']['current'] = "$base_url"; | ||
| } else { | ||
| $response['menu']['current'] = false; | ||
| } | ||
|
|
There was a problem hiding this comment.
Please split these changes into a separate PR.
| </h3> | ||
| <table border="0" cellpadding="4" cellspacing="0" width="100%" class="tabb" id="project_{{::cdash.projectid}}_{{::buildgroup.id}}"> | ||
| <!-- Build group header with rounded style --> | ||
| <div style="background: #5775b9; padding: 12px 20px; border-radius: 8px 8px 0 0; box-shadow: 0 2px 8px rgba(0,0,0,0.1); display: flex; align-items: center; justify-content: space-between; margin-top: 4px;"> |
There was a problem hiding this comment.
Please move these extensive inline styles to an external stylesheet.
|
Feel free to close this MR if you are working on the redoing of the main page. |
2 participants
Trying to improve the layout of the main CDash page by