Problem/Motivation

The multi-checkbox category dropdown filter's markup is atrocious. It is a pile of DIVs and checkboxes that are not semantically arranged like form elements, and that means the tests are extremely flaky when interacting it because they have to do bullshit like this:

    $second_category_filter_selector = '.pb-filter__multi-dropdown .pb-filter__checkbox-label:nth-child(2) input[type="checkbox"]';

Come on. This is not sustainable.

Let's change the markup so it's more like this:

<div><label for="category-ID"><input type="checkbox" id="category-ID" value="ID" />Category name (ID)</label></div>

Then we should be able to do this in tests, which is both more readable and probably (hopefully) more stable:

$assert_session->fieldExists('Category name (ID)')->check();

Issue fork project_browser-3485747

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

phenaproxima created an issue. See original summary.

sourav_paul’s picture

sourav_paul
Location Kolkata
commented
Assigned: Unassigned » sourav_paul

pfrilling made their first commit to this issue’s fork.

pfrilling opened merge request !614

pfrilling’s picture

pfrilling
Location Minster, OH
commented
Issue tags: +#nedcamp2024

Looking into this at NEDCamp. Adding the issue tag.

chrisfromredfin’s picture

chrisfromredfin
he/him/his
Primary language English
Location Portland, Maine
commented
Issue tags: +core-mvp, +beta blocker
pfrilling’s picture

pfrilling
Location Minster, OH
commented
Status: Active » Needs review

The markup changes have been completed and the testing is all passing. This is ready for review.

chrisfromredfin’s picture

chrisfromredfin
he/him/his
Primary language English
Location Portland, Maine
commented
Status: Needs review » Needs work

fail - can't find by that selector. close, though!

    There was 1 failure:
    
    1)
    Drupal\Tests\project_browser\FunctionalJavascript\ProjectBrowserUiTestJsonApi::testMultiplePlugins
    Behat\Mink\Exception\ElementNotFoundException: Element matching css
    ".pb-filter__checkbox-label-txt" not found.
pfrilling’s picture

pfrilling
Location Minster, OH
commented
Status: Needs work » Needs review

I was able to get the tests all passing. Marking this as needs review.

narendrar’s picture

narendrar
He/Him
Primary language English
Location Jaipur, India
commented
Assigned: sourav_paul » Unassigned
Issue tags: +Needs reroll

install-state.gz should not be part of MR.

pfrilling’s picture

pfrilling
Location Minster, OH
commented
Assigned: Unassigned » pfrilling
Status: Needs review » Needs work

Working on the rebase now.

pfrilling’s picture

pfrilling
Location Minster, OH
commented
Assigned: pfrilling » Unassigned
Status: Needs work » Needs review

I rebased the code and confirmed all tests are now passing. Marking as needs review.

narendrar’s picture

narendrar
He/Him
Primary language English
Location Jaipur, India
commented
Status: Needs review » Needs work

This MR needs re-roll.

shalini_jha made their first commit to this issue’s fork.

shalini_jha’s picture

shalini_jha commented

Rebased and resolved conflicts. Test changes in tests/src/FunctionalJavascript/ProjectBrowserUiTestJsonApi.php are still pending and need to be moved to ProjectBrowserUiTest::testMultiplePlugins.

shalini_jha’s picture

shalini_jha commented
Status: Needs work » Needs review

I have moved the changes from ProjectBrowserUiTestJsonApi::testMultiplePlugins to ProjectBrowserUiTest::testMultiplePlugins. I verified the output from the old testMultiplePlugins function and added the same flow to the ProjectBrowserUiTest::testMultiplePlugins.

Marking this as "Needs Review." Please review and let me know if this approach makes sense.

lostcarpark’s picture

lostcarpark commented

This is partly my fault. I fixed up the tests to work for the drop-downs. Rather than just plow in and work out selectors for the new markup, I should have asked can we make the checkboxes easier for the tests to select. I'll know better next time.

I have reviewed the changes and everything looks good to me. The revised selecters look much cleaner. I've also carried out a manual test, and everything seems to work correctly.

I note that the eslint check is failing due to the recipe.yml file. This file has not been touched by this change, so I think it's an unrelated issue, so I think it should be fixed in a separate issue.

lostcarpark’s picture

lostcarpark commented
Status: Needs review » Reviewed & tested by the community

chrisfromredfin’s picture

chrisfromredfin
he/him/his
Primary language English
Location Portland, Maine
commented
Status: Reviewed & tested by the community » Fixed

This is much more semantic, and also will help with tests!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.