-
Notifications
You must be signed in to change notification settings - Fork 35
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
fix: Modified indicators incorrectly displayed for some UI5 controls in Adaptation Project #2264
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: a73f653 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
…ub.com/SAP/open-ux-tools into fix/2263/incorrect-modified-indicators
the usage of the fl library looks fine. I will document in openui5 that they are used here |
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.
- Changeset present
- Code changes looks good
- Review comments addressed
- Tested locally
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.
Change looks good
Coverage is good
Did not test manually
{text} | ||
</Link> | ||
</Stack.Item> | ||
{text && ( |
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.
text
should always be defined and not an empty string.
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.
Since we use a control group for the new 'control' kind, we do not have the controlName
. And controlName
is taken from change.selector.type
, which for 'renameLabel' and 'addXML' unfortunately does not exist. Therefore, the only way to use the ControlGroup.tsx for kind: 'control'
is to hide the text stack item (group header) because we do not have one for this change kind.
Maybe, it does not make sense to process this change in the ControlGroup
; maybe we need a third group like ControlChange
, then the ChangeStack.tsx will have three options for rendering the change.
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.
Fixed with: 643100c
const controlChange: ControlSavedChange = { | ||
...unknownChange, | ||
kind: 'control', | ||
controlId: selectorId ?? '' |
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.
??
operator seems unnecessary here as we already check for selectorId
before.
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.
Removed in: 65d7d2f
@@ -117,6 +117,7 @@ describe('SelectionService', () => { | |||
timestamp: 1640106817301 | |||
}, | |||
{ | |||
changeType: 'propertyChange', |
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.
This looks wrong. unknown
change should not have change type propertyChange
. It seems that the unknown
also need to be change to property
.
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.
Fixed with: 0d505ba
@@ -274,6 +275,7 @@ describe('SelectionService', () => { | |||
timestamp: 1640106817301 | |||
}, | |||
{ | |||
changeType: 'propertyChange', |
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.
Same as above.
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.
Fixed with: 0d505ba
timestamp?: number; | ||
} | ||
|
||
export interface ControlSavedChange { |
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.
This word order sounds quite strange. It would be better if it matched other change type names e.g PendingControlChange
and SavedPropertyChange
.
export interface ControlSavedChange { | |
export interface SavedControlChange { |
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.
Renamed in: 65d7d2f
private async handleCommand(command: FlexCommand, inactiveCommandCount: number, index: number): Promise<void> { | ||
const pendingChange = await this.prepareChangeType(command, inactiveCommandCount, index); | ||
if (pendingChange) { | ||
this.pendingChanges.push(pendingChange); |
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 seems to be an issue with pending changes duplicating on each event. It is probably because pendingChanges
field is not reset and we always process all the changes in the stack.
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.
Fixed with: c673d03
…ub.com/SAP/open-ux-tools into fix/2263/incorrect-modified-indicators
Quality Gate passedIssues Measures |
Fix for #2263.