-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Bug fix for published labels #13747
Bug fix for published labels #13747
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## 5.x #13747 +/- ##
=========================================
Coverage 61.62% 61.62%
Complexity 34145 34145
=========================================
Files 2245 2245
Lines 102077 102077
=========================================
+ Hits 62909 62910 +1
+ Misses 39168 39167 -1
|
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.
Fine from the code perspective 👍
{% set status = entity.getPublishStatus() %} | ||
{% set labelText = ('mautic.core.form.' ~ (status == 'published' ? 'active' : 'inactive'))|trans %} | ||
<span title="{{ labelText }}" aria-label="{{ labelText }}" class="pa-10 bg-{{ labelColor }} mt-3 label label-{{ labelColor }}"> </span> |
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'd prefer to show the text so people would know what this red square means, but a title is good enough I guess.
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.
The problem is that some entities are generic
So it’ll display “Active”, for example, for something you create to be available
this inconsistency could cause confusion, but the title is there to indicate Active or Inactive if someone gets in doubt about what it means
Not a perfect solution, but I’m technically unable to solve the entities differentiation
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.
The label is already in the title and that's important for accessibility. So if it's good for visually impaired users then it should be OK for every one else, right?
Another idea: How about to use some icons here? Something like play/pause icons.
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.
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.
Works as described and looks good to me 👍
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.
Description:
This PR fixes the labels statuses. Opted to remove the text label and use only colors since now we have active/available statuses and Mautic code not always differentiate them
Steps to test this PR: