-
-
Notifications
You must be signed in to change notification settings - Fork 144
BUGFIX: Use correct color for timed visibility icon #4003
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
base: 9.0
Are you sure you want to change the base?
Conversation
const hasTimeableNodeVisibility = node?.properties?._hasTimeableNodeVisibility; | ||
|
||
if (hasTimeableNodeVisibility) { | ||
const circleColor = isDisabled ? 'error' : 'primaryBlue'; |
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 internal logic of timable node visibility has changed. That's why this also has changed. The state is rather depending on the actual visibility state (disabled/enabled) than the "calculated" state based on the datetime.
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.
But that also means, the editor has to wait for the job to run so they can see the real state.
What happens if they set hidden manually, the job would then change the value, correct?
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.
Yes, correct. As I wrote in the issue:
IIRC we discussed a feature for the UI to show a warning if the current state is not correct (e.g node is visible, but the enableAfter is in future etc). But I couldn't find any issue for this.
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.
We could use this change for that, as it would be more or less the same code ?
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 guess so. Would be a feature and we would need some idea how we want to display that.
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.
But I think your solution does not cover all scenatios atm. Please check the tests in the neos/timeable-node-visibility
package: https://github.com/neos/timeable-node-visibility/blob/9.0/Tests/Behavior/Features/HandleExceeded.feature
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.
And the backend implementation to determine the state: https://github.com/neos/timeable-node-visibility/blob/9.0/Classes/Service/TimeableNodeVisibilityService.php#L129-L154
What I did
This fixes a regression introduced with 43efd9c
Resolves: #3997
How I did it
Readded some of the necessary code from 8.3 and adjusted it to the new visibility property names.
How to verify it