Display print success #2084
Display print success #2084
Conversation
This reverts commit 8506e4a.
Display print success
Fix compile error
Display print success
font awesome changes
|
@UnchartedBull sorry for the manual build action slipping in on this PR. You might find it useful, I use it to make the deb files since electron complains about packaging on my windows. It builds, packages, and uploads as an artifact to the action run. |
There was a problem hiding this comment.
PR looks good, just some minor adjustments. Please also remove the .vs folder and all it's contents from this PR (feel free to add it to the .gitignore if you want to keep them locally).
Also do the included svg icons get used at all? As far as I can see they're not used anymore, but you're now using fontawesome (really nice btw.).
And I guess the last thing: If you want to create future PRs (especially for UI stuff) it would be super neat to include a screenshot if possible, as this makes reviewing a lot easier.
Once again thanks for your contribution! Once the few remarks have been addressed this can be merged and included in the next release:`)
src/app/files/files.component.html
Outdated
| @@ -68,7 +68,13 @@ | |||
| [matRippleUnbounded]="false" | |||
| class="files__object" | |||
| > | |||
| <img src="assets/object.svg" class="files__icon" /> | |||
| <div class="files__icon_wrapper {{file.successfull}}"> | |||
There was a problem hiding this comment.
Can we use ngClass for the file.successfull? https://angular.io/api/common/NgClass
src/app/files/files.component.html
Outdated
| [icon]="['fas', file.successfullIcon]" | ||
| class="custom-actions__action-icon" | ||
| ></fa-icon> | ||
| <img src={{file.thumbnail}} class="files__icon" /> |
There was a problem hiding this comment.
| <img src={{file.thumbnail}} class="files__icon" /> | |
| <img [src]="file.thumbnail" class="files__icon" /> |
src/app/files/files.component.html
Outdated
| @@ -78,7 +84,7 @@ | |||
| {{ file.printTime }}<span class="files__info-unit" i18n="@@files-h">h</span> | |||
| </span> | |||
| </div> | |||
| <span class="files__name"> | |||
| <span class="files__name" > | |||
There was a problem hiding this comment.
| <span class="files__name" > | |
| <span class="files__name"> |
| successfull: fileOrFolder.prints != null ? fileOrFolder.prints.last.success ? 'files__object_success' : 'files__object_failed' : 'files__object_unknown', | ||
| successfullIcon: fileOrFolder.prints != null ? fileOrFolder.prints.last.success ? 'check-circle' : 'minus-circle' : 'circle', | ||
| thumbnail: fileOrFolder.thumbnail ? this.configService.getApiURL(fileOrFolder.thumbnail, false) : 'assets/object.svg', | ||
| printTime: this.conversionService.convertSecondsToHours( | ||
| fileOrFolder.gcodeAnalysis.estimatedPrintTime, | ||
| ), | ||
| filamentWeight: this.conversionService.convertFilamentLengthToWeight( | ||
| _.sumBy(_.values(fileOrFolder.gcodeAnalysis.filament), tool => tool.length), | ||
| ), |
There was a problem hiding this comment.
I think this needs to be indented one tab less.
|
@jneilliii the manual build file is actually a nice addition! Thanks for including that! |
…into manual_build
|
@UnchartedBull I think this one is good to go now with requested changes.
|
UnchartedBull
force-pushed
the
Display-print-success
branch
from
51503f3
to
77922df
Compare
|
Ok pipeline is green now and code looks good as well, so this can be merged. I did some experimenting and I kinda like the coloured bars on the left side (similar to how notifications look like). I don't want to hijack anything here, but I thought, that it might be a good idea to post this here for discussion. Please let me know what you think. I have the version with the bars ready on my local dev system, so there won't be any coding effort difference between the two of them.
|
|
I've experienced this issue before from an accessibility stand point @UnchartedBull color alone is not good enough for those people that have color blindness, specifically red/green. I personally would say to stick with the icons, but this is your project and totally up to you. |
|
I agree color blindness could be an issue. I like them both but icons would be easier to tell them apart.
Martin Dempsey
…________________________________
From: jneilliii ***@***.***>
Sent: Thursday, August 12, 2021 3:03:27 AM
To: UnchartedBull/OctoDash ***@***.***>
Cc: Seculo9112 ***@***.***>; Author ***@***.***>
Subject: Re: [UnchartedBull/OctoDash] Display print success (#2084)
I've experienced this issue before from an accessibility stand pint @UnchartedBull<https://github.com/UnchartedBull> color alone is not good enough for those people that have color blindness, specifically red/green. I personally would say to stick with the icons, but this is your project and totally up to you.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub<#2084 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AU44WASUL6BT7RHYNIR3DMLT4KGL7ANCNFSM5A3E6OGQ>.
Triage notifications on the go with GitHub Mobile for iOS<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android<https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email>.
|
|
Yeah color blindness is a good point. The main issue i see with the icons is that they could be hard to see on smaller screens (3.5"). I think the bars would be easier in that case. I'd like to go with the color bars for now and if any issues occur switch back to the icons (good thing, that git keeps all the history :D). Thanks for your contribution once again! This will be released with the next release. |
|
Maybe if it were a configurable option between the two? I know from the discussion on Discord, one of the users that was involved does have color blindness. |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Displays print success or thumbnail in file menu
Closes #1339.