Skip to content

Conversation

@doninAtwix
Copy link
Contributor

@doninAtwix doninAtwix commented Mar 4, 2022

Description (*)

Added additional fields to UCT configuration dialog. When you click on checkbox "Add additional path to analyse" an additional field "Path To Analyse" will become available for selection.

Screenshot 2022-03-04 at 19 20 50

Fixed Issues (if relevant)

  1. Fixes [UCT] Cover themes scanning/reporting for the Tools > Run The Upgrade Compatibility Tool #1023

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with integration/functional tests (if applicable)
  • All automated tests passed successfully (all builds are green)

Copy link
Collaborator

@bohdan-harniuk bohdan-harniuk left a comment

Choose a reason for hiding this comment

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

Hello, @doninAtwix!

Please, check my notices.

Regards,

import javax.swing.JLabel;
import javax.swing.JPanel;
import javax.swing.KeyStroke;
import javax.swing.*;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please, do not use wildcard imports.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your code review. I fixed. Please check.

private JLabel modulePathError;//NOPMD
private JLabel enableComment;//NOPMD
private JLabel enableCommentPath;//NOPMD
private JCheckBox hasAdditionalPath;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please, keep labels separately from other elements. Move this one ☝️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your code review. I fixed. Please check.

final String basePath = Settings.getMagentoPath(project);

if (basePath != null) {
additionalPath.getComponent().setText(basePath);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why did you do that? What is business goal here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @bohdan-harniuk , the functionality was duplicated as for the field modulePath so that you can specify an additional path

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your code review. I fixed. Please check.

}

private void refreshAdditionalFields(final boolean isEnabled) {
additionalPath.setEnabled(isEnabled);
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can add additionalPath.setVisible(isEnabled);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your code review. I fixed. Please check.

setTitle(ConfigureUctAction.ACTION_NAME);
getRootPane().setDefaultButton(buttonOk);

hasAdditionalPath.addActionListener(event -> refreshAdditionalFields(hasAdditionalPath.isSelected()));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please, call this method refreshAdditionalFields in the constructor. So we will get saved state of this component.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Pay attention, this method should be called after setDefaultValues.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your code review. I fixed. Please check.

@doninAtwix
Copy link
Contributor Author

Updated logic, now when you click on checkbox "Add additional path to analyse" an additional field "Path To Analyse" has show/hide effect.

Screenshot 2022-03-05 at 19 39 50

Screenshot 2022-03-05 at 19 40 10

@bohdan-harniuk bohdan-harniuk changed the title 1023: Added additional fields for UCT configuration dialog 1023: Added an additional path field for UCT configuration dialog (to cover theme folder scanning) Mar 7, 2022
Copy link
Collaborator

@bohdan-harniuk bohdan-harniuk left a comment

Choose a reason for hiding this comment

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

Great work!
Thank you, @doninAtwix!

@bohdan-harniuk bohdan-harniuk merged commit 267b88c into magento:mainline/uct-1023-theme-scanning-feature Mar 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants