Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add cancel button to variable name creation dialog #1

Merged
merged 1 commit into from
May 20, 2024
Merged

Add cancel button to variable name creation dialog #1

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions .../nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/js/nf/canvas/nf-variable-registry.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1620,7 +1620,22 @@
click: function () {
addNewVariable();
}
Comment on lines 1620 to 1622
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The addNewVariable function is called without any arguments or context provided, which could potentially lead to issues if the function relies on specific arguments or this context. It's important to ensure that functions are called with the necessary context and parameters, especially in event handlers where the context might not be as expected. Consider verifying the definition of addNewVariable to ensure it is being called correctly and, if necessary, adjust the call to provide the required context or parameters.

},

},
{
buttonText: 'Cancel',
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: Consider using a more descriptive button text.

The button text 'Cancel' is clear, but it might be more user-friendly to use a more descriptive text like 'Cancel Changes' or 'Discard'. This can help users understand the action better.

Suggested change
buttonText: 'Cancel',
buttonText: 'Cancel Changes',

color: {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: Consider using a centralized color palette.

Instead of hardcoding the color values here, consider using a centralized color palette or theme. This will make it easier to maintain and ensure consistency across the application.

Suggested change
color: {
color: {
base: theme.colors.base,
hover: theme.colors.hover,

base: '#E3E8EB',
hover: '#C7D2D7',
text: '#004849'
},
handler: {
click: function () {
this.modal('hide');
Comment on lines +1634 to +1635
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The use of this within the anonymous function for the click handler may not refer to the expected context, potentially leading to runtime errors when attempting to call modal('hide'). In JavaScript, the value of this inside an anonymous function does not automatically refer to the object that contains the function. To ensure the correct context is used, consider using arrow functions or binding the function to the correct context.

}
}

}],
handler: {
close: function () {
Expand Down