-
Notifications
You must be signed in to change notification settings - Fork 28
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
Add flag to render only job name #402
base: main
Are you sure you want to change the base?
Conversation
Thanks for opening this pull request! Please check out our contributing guidelines! |
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.
- If you are changing the switch label from Show job name only to Show only job name, then change the variable name accordingly.
- Add unit tests for your changes
- Use conventional commit types for commit messages. Refer https://github.com/otto-de/gitactionboard/blob/main/CONTRIBUTING.md#types
<v-card-item class="pt-0 pb-0"> | ||
<v-switch | ||
v-model="showJobNameOnly" | ||
label="Show job name only" |
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 can change it to "Show only job name"
@@ -99,7 +108,9 @@ export default { | |||
enableMaxIdleTimeOptimization: preferences.enableMaxIdleTimeOptimization, | |||
themeInstance, | |||
isDirty: false, | |||
showBuildsDueToTriggeredEvents: getShowBuildsDueToTriggeredEvents() | |||
showBuildsDueToTriggeredEvents: getShowBuildsDueToTriggeredEvents(), | |||
nameTokens: preferences.nameTokens, |
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.
is this required? I haven't found any usage of it
const parts = data.name.split(' :: '); | ||
return { | ||
...data, | ||
name: preferences.showJobNameOnly ? parts[parts.length - 1] : data.name | ||
}; |
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.
It'll be better to pull out this as mapper. Depends on the configuration use required mapper.
Something like:
const jobNameMapper = (data) => {
const parts = data.name.split(' :: ');
return {
...data,
name: parts[parts.length - 1]
}
}
const identityMapper = data => data;
const mapper = preferences.showJobNameOnly ? jobNameMapper : identityMapper;
frontend/src/services/apiService.js
Outdated
export const fetchConfig = (isMockEnv = false) => | ||
isMockEnv ? Promise.resolve({}) : fetchJsonContent(preparePath('/config')); |
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.
Why is this changes required?
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.
running locally with mock data didn't work without this change, but you are write it is out of scope for this PR, will remove 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.
I am not sure, have you tried the command mentioned on https://github.com/otto-de/gitactionboard/blob/main/CONTRIBUTING.md#run-only-frontend-with-mock-data ?
Co-authored-by: Anna Kvashchuk <[email protected]>
Hi @sumanmaity1234 Thanks a lot for your review! We have now addressed all the comments, and made requested 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.
Hi @kvashchuka, I provided a small comment, could you please take care of it?
@@ -50,13 +50,24 @@ export default { | |||
isIdleHealthyBuild(lastBuildStatus, activity) { | |||
return lastBuildStatus === 'Success' && activity === 'Sleeping'; | |||
}, | |||
mapNameIfPreferred(data) { | |||
const parts = data.name.split(' :: '); |
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.
Can we split the name only if required
Hi @kvashchuka, I was thinking about this feature. The current solution really works well for single repository but once user has multiple repository, this solution might not add much value. One idea could be, allow user to configure which part they are interested, some might want to hide the workflow name, some might repository or combinations of it. |
hi @sumanmaity1234, as @kvashchuka is gone for a few weeks, they've asked if I could pick up the mantle on this. I'll look into implementing your suggestion. however, would it be possible for me to use this feature to look into a new layout for the cards? |
Hi @aronhoyer, sure, feel free to update the design. |
Hi @aronhoyer, overall design looks interesting but couple of things I think you overlook
Addition to all the above mentioned points, we need to make sure that good numbers of cards/jobs are displayed on the screen without scrolling, so we need to choose height and width accordingly. If the above points can be easily incorporated in the design then it'll really nice from the product point of view |
just a general note: all four rows explore different alternatives to the card design and how to display the job status. the designs would not appear at the same time. not sure whether that was unclear or not.
yeah I meant to write repo name instead of branch name 😅 that's my b
that's why I thought splitting up the parts that make up the formatted name would be beneficial. we could also do things like truncating the job name if it exceeds two lines or something.
not a problem at all. maybe, to not clutter the ui too much if there are many jobs displayed on the dashboard, the show/hide button could appear once the user hovers the card?
not a problem either |
I think it'll good to keep the job name as it is (at least for the first try) and maybe we can truncate the repo name and workflow name.
Not sure how it'll work as somehow we have make it prominent that on click on the card it'll open on GitHub. |
created a Figma prototype https://www.figma.com/proto/kain0iyc8Cf0zkbV5QoOD6/Untitled?page-id=0%3A1&type=design&node-id=20-83&viewport=238%2C401%2C0.71&t=qNZ3OIAsKstmRDla-1&scaling=min-zoom&mode=design
good shout. I guess the card should have some hover effect |
Hi @aronhoyer,
|
@sumanmaity1234 hi! sorry I went awol for a bit. fell ill and this kinda fell to the wayside
that was the idea. either that or a looping progress bar type animation.
won't that just be the first card in the list? |
Hi @aronhoyer, I hope you are better now.
Sure, that make sense. I just wanted to clarify my doubt.
Unable to understand what do you meant by "first card". In the dashboard, you'll have single grid/box for each job. My question was related to the last run status for a job. For example, in the following image, you can see the current jobs are running but it also indicate the last run status, for job 1 ( Note: screenshot is from https://otto-de.github.io/gitactionboard/#/workflow-jobs |
Hi @sumanmaity1234! I think this PR lost traction as the scope grew larger. Could we please go back a step and reconsider merging the PR with the original intent (aka simply have a flag to only show the job name)? I do understand that it is not a perfect solution for every user, but it makes a big difference for the usage of gitaction board for my team and I believe it does not have any negative impact on other users. |
Hi @kvashchuka, could you please check if your changes works with the latest codebase? Also please add a screenshot how will it look post your changes. Additionally, there are couple of code change requested. Once these changes are done, I'll merge the PR Thanks |
- Design is inspired by the suggestion from @aronhoyer as part of #402
When displaying action statuses of just one repo, it is unnecessary to display the repo name. The "Show job name only" switch will give a user an option to hide repo name and workflow name, leaving only the job name, thus increasing readability.