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

Drop Select2 #7701

Merged
merged 3 commits into from
Dec 15, 2023
Merged

Drop Select2 #7701

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
dfe01e3
Drop Select2
Etheryte Dec 13, 2023
d6824e1
Fix rebase
Etheryte Dec 13, 2023
ad083a6
Remove unnecessary escape
Etheryte Dec 13, 2023
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
1 change: 0 additions & 1 deletion java/code/webapp/WEB-INF/decorators/layout_error.jsp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@

<script src="/javascript/jquery.js?cb=${cb_version}"></script>
<script src="/javascript/bootstrap.js?cb=${cb_version}"></script>
<script src="/javascript/select2/select2.js?cb=${cb_version}"></script>
<script src="/javascript/spacewalk-essentials.js?cb=${cb_version}"></script>
<script src="/javascript/spacewalk-checkall.js?cb=${cb_version}"></script>

Expand Down
3 changes: 0 additions & 3 deletions java/code/webapp/WEB-INF/decorators/layout_head.jsp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,6 @@
<!-- import plugins styles -->
<link rel="stylesheet" href="/css/legacy/jquery.timepicker.css?cb=${cb_version}" />
<link rel="stylesheet" href="/css/bootstrap-datepicker.css?cb=${cb_version}" />
<link rel="stylesheet" href="/javascript/select2/select2.css?cb=${cb_version}" />
<link rel="stylesheet" href="/javascript/select2/select2-bootstrap.css?cb=${cb_version}" />

<!-- import styles -->
<c:set var="webTheme" value="${GlobalInstanceHolder.USER_PREFERENCE_UTILS.getCurrentWebTheme(pageContext)}"/>
Expand Down Expand Up @@ -71,7 +69,6 @@

<script src="/javascript/legacy/jquery.min.js?cb=${cb_version}"></script>
<script src="/javascript/legacy/bootstrap.min.js?cb=${cb_version}"></script>
<script src="/javascript/select2/select2.js?cb=${cb_version}"></script>
<script src="/javascript/spacewalk-essentials.js?cb=${cb_version}"></script>
<script src="/javascript/spacewalk-checkall.js?cb=${cb_version}"></script>
<script src="/javascript/ajax.js?cb=${cb_version}"></script>
Expand Down
14 changes: 9 additions & 5 deletions java/code/webapp/WEB-INF/pages/common/fragments/schedule-options.jspf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
<%@ taglib uri="http://struts.apache.org/tags-bean" prefix="bean" %>
<%@ taglib uri="http://rhn.redhat.com/rhn" prefix="rhn" %>

<script type="text/javascript" src="/javascript/schedule-options.js?cb=${rhn:getConfig('web.buildtimestamp')}"></script>
<div class="spacewalk-scheduler">
<div class="form-horizontal">
<c:set var="maintenanceWindowsEmpty" value="${maintenanceWindows != null && empty maintenanceWindows}" />
Expand Down Expand Up @@ -56,11 +55,16 @@
/>
<label for="schedule-by-action-chain"><bean:message key="schedule-options.action-chain"/></label>
</div>
<div class="col-sm-6">
<input type="hidden" id="action-chain" name="action_chain"
data-existing-action-chains='${existingActionChains}'
/>
<div class="col-sm-6" id="action-chain-picker">
<script type="text/javascript">
spaImportReactPage('schedule-options/action-chain-picker').then(function(module) {
module.renderer('action-chain-picker', {
actionChains: '${existingActionChains}'
});
});
</script>
</div>
<input type="hidden" id="action-chain" name="action_chain" />
</div>
</div>
</div>
1 change: 1 addition & 0 deletions java/spacewalk-java.changes.eth.select2
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
- Remove Select2
2 changes: 1 addition & 1 deletion testsuite/features/reposync/reference_srv_sync_products_extra.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ Feature: Synchronize extra products in the products page of the Setup Wizard
When I follow the left menu "Admin > Setup Wizard > Products"
And I wait until I do not see "Loading" text
And I enter "Ubuntu" as the filtered product description
And I select "amd64-deb" in the dropdown list of the architecture filter
And I select "amd64-deb" from "product-arch-filter"
Copy link
Member

Choose a reason for hiding this comment

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

Ideally, we better hide this technical detail in the step definition.
That way, if we change this id in the future, we only need to change it in the step definition and not in all the scenarios using it.

Copy link
Member

Choose a reason for hiding this comment

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

Hi @Etheryte the Gherkin syntax and Cucumber scenarios have been designed to use natural language to describe the steps a user must follow to achieve something. Ideally, we should write the scenarios as if were on the official documentation. So, even if other steps don't follow this good practice, let's try to do it properly wherever we can.

The good news is that's easy to fix, we only need to create a new step definition, where inside it we use this id.

Copy link
Contributor

@Bischoff Bischoff Dec 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, we should avoid using internals as much as we could. The "feature" part should remain "as the end user sees it", and end users don't see ids when they are in front of the UI, they see texts...

That being said, instead of adding new specialized steps, we might try to write a generic step that uses texts instead of ids and labels.

And I wait until I see "Ubuntu 22.04" text
And I select "Ubuntu 22.04" as a product
Then I should see the "Ubuntu 22.04" selected
Expand Down
2 changes: 1 addition & 1 deletion testsuite/features/reposync/srv_sync_products.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ Feature: Synchronize products in the products page of the Setup Wizard
And I wait until I do not see "Loading" text
And I enter "RHEL or SLES ES" as the filtered product description
Then I should see a "RHEL or SLES ES or CentOS 8 Base" text
When I select "x86_64" in the dropdown list of the architecture filter
When I select "x86_64" from "product-arch-filter"
Then I should see a "RHEL or SLES ES or CentOS 8 Base" text

@scc_credentials
Expand Down
24 changes: 12 additions & 12 deletions testsuite/features/secondary/srv_dist_channel_mapping.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,8 @@ Feature: Distribution Channel Mapping
Then I should see a "Create Distribution Channel Map" text
When I enter "SUSE Linux Enterprise Server 15 SP 4" as "os"
And I enter "15.5" as "release"
And I select "x86_64" from "architecture" dropdown
And I select "SLE-Product-SLES15-SP4-Pool for x86_64" from "channel_label" dropdown
And I select "x86_64" from "architecture"
And I select "SLE-Product-SLES15-SP4-Pool for x86_64" from "channel_label"
Comment on lines +28 to +29
Copy link
Member

@srbarrios srbarrios Nov 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

It would be better to hide these technical details inside new step definitions too.
Personally, I think that from the user's point of view, knowing that we are acting on a dropdown web element, if that is still the case, can help.

Copy link
Member Author

Choose a reason for hiding this comment

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

In that case, do you want to add this to all other tests that use dropdowns? We do the same for dropdowns everywhere else: And I select "equals" from "matcher", And I select "1-centos7_ssh_minion_key" from "activationKeys" etc. Currently only the steps with Select2 were different, this change makes it be the same way everywhere. Unless I misunderstand what you mean?

Copy link
Member Author

Choose a reason for hiding this comment

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

@srbarrios ping.

Copy link
Member

@srbarrios srbarrios Dec 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

OK, I think I did not get it the first time. I see, so we already have a step definition doing exactly the same, but without dropdown word appended. That's it?
In such case, in my opinion, I would better keep the step that has the word dropdown and replace the scenarios where we use the other step. The reasoning still the same, try to describe with natural language in a visual manner the actions that a user must follow.

Still, we should improve it later, and don't use element ids as step definition parameters, exposing them in the Gherkin text, instead, we should be hiding this inside the step.

Example:

When(/^I select "([^"]*)" from (activation keys|filter match|channel label) dropdown$/) do |option, dropdown|
  field =
    case dropdown
    when 'activation keys'
      'activationKeys'
    when 'filter match'
      'matcher'
    when 'channel_label'
      'channel_label'
    else
      raise ScriptError, "Unknown dropdown '#{dropdown}'"
    end
  
  if has_select?(field, with_options: [option], wait: 1)
    select(option, from: field)
  else
    # Custom React selector
    xpath_field = "//*[contains(@class, 'data-testid-#{field}-child__control')]"
    xpath_option = ".//*[contains(@class, 'data-testid-#{field}-child__option') and contains(text(), '#{option}')]"
    find(:xpath, xpath_field).click
    find(:xpath, xpath_option, match: :first).click
  end
end

Copy link
Contributor

Choose a reason for hiding this comment

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

Or see my proposal above.

And I click on "Create Mapping"
Then I should see a "SUSE Linux Enterprise Server 15 SP 4" link in the content area

Expand All @@ -37,8 +37,8 @@ Feature: Distribution Channel Mapping
Then I should see a "Create Distribution Channel Map" text
When I enter "openSUSE Leap 15.5" as "os"
And I enter "15.5" as "release"
And I select "x86_64" from "architecture" dropdown
And I select "openSUSE Leap 15.5 (x86_64)" from "channel_label" dropdown
And I select "x86_64" from "architecture"
And I select "openSUSE Leap 15.5 (x86_64)" from "channel_label"
And I click on "Create Mapping"
Then I should see a "openSUSE Leap 15.5" link in the content area

Expand All @@ -48,8 +48,8 @@ Feature: Distribution Channel Mapping
Then I should see a "Create Distribution Channel Map" text
When I enter "Ubuntu 22.04.01 LTS" as "os"
And I enter "22.04" as "release"
And I select "x86_64" from "architecture" dropdown
And I select "Fake-Base-Channel-Debian-like" from "channel_label" dropdown
And I select "x86_64" from "architecture"
And I select "Fake-Base-Channel-Debian-like" from "channel_label"
And I click on "Create Mapping"
Then I should see a "Ubuntu 22.04.01 LTS" link in the content area

Expand All @@ -60,8 +60,8 @@ Feature: Distribution Channel Mapping
Then I should see a "Create Distribution Channel Map" text
When I enter "SUSE Linux Enterprise Server 15 SP 4 iSeries" as "os"
And I enter "15.5" as "release"
And I select "iSeries" from "architecture" dropdown
And I select "Fake-Base-Channel-i586" from "channel_label" dropdown
And I select "iSeries" from "architecture"
And I select "Fake-Base-Channel-i586" from "channel_label"
And I click on "Create Mapping"
Then I should see a "SUSE Linux Enterprise Server 15 SP 4 iSeries" link in the content area

Expand All @@ -75,7 +75,7 @@ Feature: Distribution Channel Mapping
When I follow "SUSE Linux Enterprise Server 15 SP 4"
Then I should see a "Update Distribution Channel Map" text
When I enter "SUSE Linux Enterprise Server 15 SP 4 modified" as "os"
And I select "SLE-Product-SLES15-SP4-Pool for x86_64" from "channel_label" dropdown
And I select "SLE-Product-SLES15-SP4-Pool for x86_64" from "channel_label"
And I click on "Update Mapping"
Then I should see the text "SUSE Linux Enterprise Server 15 SP 4 modified" in the Operating System field
And I should see the text "sle-product-sles15-sp4-pool-x86_64" in the Channel Label field
Expand All @@ -89,7 +89,7 @@ Feature: Distribution Channel Mapping
When I follow "openSUSE Leap 15.5"
Then I should see a "Update Distribution Channel Map" text
When I enter "openSUSE Leap 15.5 modified" as "os"
And I select "openSUSE Leap 15.5 (x86_64)" from "channel_label" dropdown
And I select "openSUSE Leap 15.5 (x86_64)" from "channel_label"
And I click on "Update Mapping"
Then I should see the text "openSUSE Leap 15.5 modified" in the Operating System field
And I should see the text "opensuse_leap15_5-x86_64" in the Channel Label field
Expand All @@ -101,7 +101,7 @@ Feature: Distribution Channel Mapping
And I should see the text "sle-product-sles15-sp4-pool-x86_64" in the Channel Label field
When I follow "Ubuntu 22.04.01 LTS"
And I enter "Ubuntu 22.04.01 LTS modified" as "os"
And I select "Fake-Base-Channel-Debian-like" from "channel_label" dropdown
And I select "Fake-Base-Channel-Debian-like" from "channel_label"
And I click on "Update Mapping"
Then I should see the text "Ubuntu 22.04.01 LTS modified" in the Operating System field
And I should see the text "fake-base-channel-suse-like" in the Channel Label field
Expand All @@ -114,7 +114,7 @@ Feature: Distribution Channel Mapping
And I should see the text "fake-base-channel-i586" in the Channel Label field
When I follow "SUSE Linux Enterprise Server 15 SP 4 iSeries"
And I enter "SUSE Linux Enterprise Server 15 SP 4 iSeries modified" as "os"
And I select "Fake-Base-Channel-Debian-like" from "channel_label" dropdown
And I select "Fake-Base-Channel-Debian-like" from "channel_label"
And I click on "Update Mapping"
Then I should see the text "SUSE Linux Enterprise Server 15 SP 4 iSeries modified" in the Operating System field
And I should see the text "fake-base-channel-debian-like" in the Channel Label field
Expand Down
9 changes: 0 additions & 9 deletions testsuite/features/step_definitions/navigation_steps.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,15 +202,6 @@
end
end

# select an item from any dropdown
Copy link
Contributor

@Bischoff Bischoff Dec 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I checked, it's not used anywhere else, good.

When(/^I select "(.*?)" from "([^"]*)" dropdown/) do |selection, label|
# let the the select2js box filter open the hidden options
xpath_query = "//select[@name='#{label}']"
raise ScriptError, "xpath: #{xpath_query} not found" unless find(:xpath, xpath_query).click
# select the desired option
raise ScriptError, "#{label} #{selection} not found" unless find(:xpath, "//select[@name='#{label}']/option[contains(text(), '#{selection}')]").click
end

When(/^I select the parent channel for the "([^"]*)" from "([^"]*)"$/) do |client, from|
product_key = $is_container_provider ? 'Fake' : product
select(BASE_CHANNEL_BY_CLIENT[product_key][client], from: from, exact: false)
Expand Down
8 changes: 0 additions & 8 deletions testsuite/features/step_definitions/setup_steps.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,14 +73,6 @@
end
end

When(/^I select "(.*?)" in the dropdown list of the architecture filter$/) do |architecture|
Copy link
Contributor

Choose a reason for hiding this comment

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

same, i checked it is not used anywhere else, good

# let the the select2js box filter open the hidden options
xpath_query = '//div[@id=\'s2id_product-arch-filter\']/ul/li/input'
raise ScriptError, "xpath: #{xpath_query} not found" unless find(:xpath, xpath_query).click
# select the desired option
raise ScriptError, "Architecture #{architecture} not found" unless find(:xpath, "//div[@id='select2-drop']/ul/li/div[contains(text(), '#{architecture}')]").click
end

When(/^I (deselect|select) "([^"]*)" as a product$/) do |select, product|
# click on the checkbox to select the product
xpath = "//span[contains(text(), '#{product}')]/ancestor::div[contains(@class, 'product-details-wrapper')]/div/input[@type='checkbox']"
Expand Down
34 changes: 0 additions & 34 deletions web/html/javascript/schedule-options.js
View file
Open in desktop

This file was deleted.

42 changes: 27 additions & 15 deletions web/html/src/branding/css/base/visualization.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,6 @@
display: inline-block;
}

.no-bold {
font-weight: normal!important;
}

.inline-icon {
i {
width: 1em;
Expand Down Expand Up @@ -49,7 +45,6 @@

#visualization-filter-wrapper {
display: none;
overflow-y: auto;
background: lighten(@gray-light, 8%);
font-size: 0.9em;
border: 1px solid #ddd;
Expand Down Expand Up @@ -83,10 +78,9 @@
vertical-align: top;
padding: 3px 10px;
> .filter-title {
max-width: 250px;
font-weight: bold;
padding: 4px 2px;
margin-right: 4px;
margin-bottom: 4px;
}
label {
font-weight: normal;
Expand All @@ -108,14 +102,6 @@
margin-right: 5px;
vertical-align: sub;
}
.select2-container-multi .select2-choices .select2-search-field input {
height: 28px;
padding: 2px 4px;
}
.select2-container-multi .select2-choices .select2-search-choice {
margin-top: 4px;
margin-bottom: 1px;
}
.toggle-grouping-level i{
margin: 0px 4px;
vertical-align: middle;
Expand All @@ -124,6 +110,32 @@
.grpCriterion {
margin-top: 1px;
}

.btn-group {
margin-top: 8px;
display: block;
}

.combobox {
display: flex;
align-items: center;
gap: 8px;
width: 250px;
margin-top: 4px;

> form {
flex: 1;
display: block;

.form-group {
margin: 0 !important;
}

.help-block {
display: none !important;
}
}
}
}
select {
width: 100%;
Expand Down
33 changes: 0 additions & 33 deletions web/html/src/branding/css/susemanager/components/inputs.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,36 +50,3 @@ select[size] {
.input-group-addon {
border-color: @eos-bc-gray-700 !important;
}

.select2-container {
padding: 0 !important;
}

.select2-choices {
border-radius: 0 !important;
border: none !important;
outline: none !important;
box-shadow: none !important;
}

.select2-search-field {
width: 100%;
}

.select2-input {
width: 100% !important;
height: @input-height !important;
padding: 8px !important;
border: 1px solid @eos-bc-gray-700 !important;
border-radius: @border-radius-base !important;

.select2-container-active & {
border-color: @eos-bc-blue-500 !important;
outline: none !important;
}
}

.select2-drop-active {
border-color: @eos-bc-blue-500 !important;
border-radius: 0 !important;
}
8 changes: 0 additions & 8 deletions web/html/src/build/webpack.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,14 +64,6 @@ module.exports = (env, argv) => {
to: path.resolve(__dirname, "../dist/javascript/legacy"),
},
// TODO: Take only what we need after we've confirmed it works fine, otherwise there's a lot of fluff in this
{
from: path.resolve(__dirname, "../node_modules/select2"),
to: path.resolve(__dirname, "../dist/javascript/legacy/select2"),
},
{
from: path.resolve(__dirname, "../node_modules/select2-bootstrap-css/select2-bootstrap.css"),
to: path.resolve(__dirname, "../dist/javascript/legacy/select2"),
},
{
from: path.resolve(__dirname, "../node_modules/timepicker/jquery.timepicker.js"),
to: path.resolve(__dirname, "../dist/javascript/legacy"),
Expand Down
2 changes: 1 addition & 1 deletion web/html/src/components/action-schedule.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,7 +283,7 @@ class ActionSchedule extends React.Component<ActionScheduleProps, ActionSchedule
id="action-chain"
name="action_chain"
selectedId={this.state.actionChain?.id}
data={this.state.actionChains}
options={this.state.actionChains}
onSelect={this.onSelectActionChain}
onFocus={this.onFocusActionChain}
/>
Expand Down
13 changes: 9 additions & 4 deletions web/html/src/components/combobox.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,14 @@ export type ComboboxItem = {
};

type ComboboxProps = {
id: string;
name: string;
data?: Array<ComboboxItem>;
id?: string;
name?: string;
options?: Array<ComboboxItem>;
selectedId?: (number | null | undefined) | (string | null | undefined);
onFocus?: () => void;
onSelect: (value: ComboboxItem) => void;
getNewOptionData?: (userInput: string, label: string) => { id: string; value: string; label: string };
placeholder?: string;

/** Id for testing purposes */
"data-testid"?: string;
Expand Down Expand Up @@ -74,7 +76,7 @@ export class Combobox extends React.Component<ComboboxProps, ComboboxState> {

// The react-select is expecting the value to be a string, but let's keep the original id here so we can propagate
// correctly the selected option up.
const options: Array<ReactSelectItem> = (this.props.data || []).map((item) => ({
const options: Array<ReactSelectItem> = (this.props.options || []).map((item) => ({
value: item.id.toString(),
id: item.id,
label: item.text,
Expand All @@ -91,6 +93,9 @@ export class Combobox extends React.Component<ComboboxProps, ComboboxState> {
options={options}
styles={colourStyles}
menuPortalTarget={document.body}
formatCreateLabel={(label: string) => t("Create {label}", { label })}
getNewOptionData={this.props.getNewOptionData}
placeholder={this.props.placeholder}
{...testAttributes}
/>
);
Expand Down
Loading
Loading