Skip to content
Snippets Groups Projects

#3470540: "Create some front end"

7 unresolved threads

Merge request reports

Merge request pipeline #323041 passed with warnings

Merge request pipeline passed with warnings for 0f1fb660

Approval is optional
Code Quality is loading
Test summary results are being parsed

Merged by Chris WellsChris Wells 8 months ago (Oct 29, 2024 8:17pm UTC)

Merge details

  • Changes merged into 2.0.x with bbf8a47d (commits were squashed).
  • Did not delete the source branch.

Pipeline #324458 passed with warnings

Pipeline passed with warnings for bbf8a47d on 2.0.x

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
9 9 */
10 10 final class BooleanFilter extends FilterBase {
11 11
12 public function __construct(public bool $value, mixed ...$arguments) {
12 public function __construct(public bool $value, public readonly array $options, mixed ...$arguments) {
  • :thumbsdown: BooleanFilter should not behave like a set of options. If anything, this should simply allow for a distinct "on label" and "off label", like this:

    Suggested change
    12 public function __construct(public bool $value, public readonly array $options, mixed ...$arguments) {
    12 public function __construct(public bool $value, public readonly string|\Stringable $off_label, public readonly string|\Stringable $on_label, mixed ...$arguments) {

    That said, the intention of BooleanFilter was to display as a single checkbox, so I'm not sure you even need to do this. On the frontend, you could render it like this (pseudo-code):

    <input type="radio" value="1" />${filter_name}
    <input type="radio" value="0" />${Drupal.t('Show all')}

    But having BooleanFilter accept an array of options like this is a misunderstanding of what BooleanFilter is intended to do, so I think we need to correct that before this can proceed.

    Edited by Adam G-H
  • Kunal Sachdev changed this line in version 11 of the diff

    changed this line in version 11 of the diff

  • Please register or sign in to reply
  • 252 filterType="developmentStatus"
    253 changeHandler={onAdvancedFilter}
    254 let:id
    255 let:label
    256 >
    257 <label
    258 slot="label"
    259 class="search__checkbox-label"
    260 for={`developmentStatus${id}`}
    261 >
    262 {label}
    263 </label>
    264 </FilterGroup>
    265 {/if}
    214 {#each Object.entries($sourceFilters) as [type, filter]}
    215 {#if filter._type === 'boolean'}
  • 256 >
    257 <label
    258 slot="label"
    259 class="search__checkbox-label"
    260 for={`developmentStatus${id}`}
    261 >
    262 {label}
    263 </label>
    264 </FilterGroup>
    265 {/if}
    214 {#each Object.entries($sourceFilters) as [type, filter]}
    215 {#if filter._type === 'boolean'}
    216 <BooleanFilter
    217 filterTitle={filter.name}
    218 filterData={filter.options}
    219 filterType={type}
  • Kunal Sachdev added 10 commits

    added 10 commits

    • f7049e1c...50e70d15 - 3 commits from branch project:2.0.x
    • e299bd3d - Rename FilterGroup to BooleanFilter and some refactoring
    • 4a5cbba0 - use prettier
    • d9b62ed4 - refactoring in progress
    • 6fd5306d - change the code related to rendering of sourcefilters
    • 7bde48fc - change getFilterDefinitions() in remaining places
    • b8f1a476 - change getFilterDefinitions() in remaining place
    • 4b89c1f6 - Rename Filter to MultipleChoiceFilter

    Compare with previous version

  • Kunal Sachdev added 1 commit

    added 1 commit

    • c4d0c8c2 - remove $options from BooleanFilter

    Compare with previous version

  • Kunal Sachdev added 1 commit

    added 1 commit

    Compare with previous version

  • Kunal Sachdev added 1 commit

    added 1 commit

    Compare with previous version

  • Kunal Sachdev added 1 commit

    added 1 commit

    Compare with previous version

  • Kunal Sachdev added 1 commit

    added 1 commit

    Compare with previous version

  • Kunal Sachdev added 15 commits

    added 15 commits

    Compare with previous version

  • Kunal Sachdev added 13 commits

    added 13 commits

    Compare with previous version

  • Kunal Sachdev added 1 commit

    added 1 commit

    • 038e349c - change variable names in BooleanFilter

    Compare with previous version

  • Kunal Sachdev added 1 commit

    added 1 commit

    • dc9f6482 - fixing test failures in progress

    Compare with previous version

  • Adam G-H added 4 commits

    added 4 commits

    Compare with previous version

  • 6 5 export let changeHandler;
    7 export let filterType;
    6 export let type;
    7 export let description;
    8 const { Drupal } = window;
    8 9 </script>
    9 10
    10 11 <div class="filter-group__filter-options form-item">
    11 <label for={filterType} class="form-item__label">{filterTitle}</label>
    12 <label for={type} class="form-item__label">{name}</label>
    12 13 <select
    13 name={filterType}
    14 name={type}
    14 15 class="search__filter-select form-select form-element form-element--type-select"
    15 bind:value={$filters[filterType]}
    16 bind:value={$filters[type]}
    • Based on what I'm seeing in Svelte's docs, this should work. The fact that it doesn't, as proven by the tests, suggests that something odd is happening. I wonder if it's a type mismatch? Maybe $filters[type] === 0, but the option value is '0' (i.e., zero as a string). That, I bet, would break it, based on what I see in the Svelte docs about this:

      A value binding corresponds to the value property on the selected , which can be any value (not just strings, as is normally the case in the DOM).

    • Please register or sign in to reply
  • Adam G-H added 1 commit

    added 1 commit

    Compare with previous version

  • Kunal Sachdev added 1 commit

    added 1 commit

    • 89c69569 - remove $description from FilterBase and $on_label and $off_label in BooleanFilter

    Compare with previous version

  • Kunal Sachdev added 19 commits

    added 19 commits

    Compare with previous version

  • Kunal Sachdev added 1 commit

    added 1 commit

    • 303fb7a1 - fix unit test defined for filters

    Compare with previous version

  • Kunal Sachdev added 1 commit

    added 1 commit

    Compare with previous version

  • Kunal Sachdev added 1 commit

    added 1 commit

    • 802f8ced - change ids of status filters and fix tests

    Compare with previous version

  • 236 236 ]);
    237 237
    238 238 // Make sure the second filter applied is the security covered filter.
    239 $this->assertEquals('Covered by a security policy', $this->getElementText(self::SECURITY_OPTION_SELECTOR . self::OPTION_CHECKED));
    239 $option = $assert_session->optionExists('securityCoverage', '1');
    240 $this->assertSame('Show projects covered by a security policy', $option->getText());
    241 $this->assertTrue($option->isSelected());
  • Kunal Sachdev added 24 commits

    added 24 commits

    Compare with previous version

  • Kunal Sachdev added 2 commits

    added 2 commits

    • e3c3169d - better comment for the testing block
    • ac169540 - fix order of the defined source filters

    Compare with previous version

  • Kunal Sachdev added 25 commits

    added 25 commits

    Compare with previous version

  • 11 11 */
    12 12 enum DevelopmentStatus: string {
    13 13
    14 case Active = 'active';
    15 case All = 'all';
    14 case Active = '1';
    15 case All = '0';
    • Comment on lines +14 to +15

      This is honestly a little bit odd, but I recognize why we need it for now. The enum provides a good layer of abstraction, so we can always change these values later if we have to. :thumbsup:

    • Please register or sign in to reply
  • Adam G-H
  • Adam G-H
  • 176 176 'Assert second category is selected.',
    177 177 );
    178 178
    179 // Press space to expand categories drop-down again.
    179 // Press tab to navigate to next drop-down.
  • Kunal Sachdev added 26 commits

    added 26 commits

    Compare with previous version

  • merged

  • Please register or sign in to reply
    Loading