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.

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

add selection #13

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

add selection #13

wants to merge 14 commits into from

Conversation

Amerlander
Copy link
Contributor

@Amerlander Amerlander commented Feb 21, 2021

Introducing a filter for selecting items additional to filter and disable.
This gives selected items an class, which could be styled from outside.

Actually I keept selected as class name, which was used for active/focused items. So this might be a breaking change and I would totally undertand if you dislike this.
For the former selected class I introduced active and changed the styling to a light blue border.

Here is the code where I use all functions, if you think this change is reasonable I will also write an update for the readme:

<script>
    import Typeahead from "svelte-typeahead";
    import { mdFieldsAll } from './stores.js';

    function handleSelect(e) {   
        let i = e.detail.originalIndex;
        $mdFieldsAll[i].selected = !$mdFieldsAll[i].selected;
    }

    const extract = (item) => item.name_human_readable + ' ['+ item.title +']';

    const disable = (item) => item.required;

    const selection = (item) => item.selected;

    const filter = (item) => item.hidden;

  </script>

<div data-svelte-mycomponent>
{#await $mdFieldsAll}
  <p>...waiting</p>
{:then data}
    <Typeahead {data} {extract} {selection} {disable} {filter} inputAfterSelect='keep' focusAfterSelect='true' let:result on:select="{handleSelect}">
        <div>
            <h3>{@html result.string}</h3>
            <p>{@html result.original.explica}</p>
        </div>
    </Typeahead>
{:catch error}
  <p style="color: red">{error.message}</p>
{/await}
</div>

<style>
// overwrite default styling of selected items
    [data-svelte-mycomponent] :global([data-svelte-typeahead] .selected) {
        background: rgb(200,260,255);
    }
</style>

I further changed the global css classes a bit, to keep their scope in the component and its sub-components:
:global([data-svelte-search] label)
to
[data-svelte-typeahead] :global([data-svelte-search] label)

Copy link
Owner

@metonym metonym left a comment

Choose a reason for hiding this comment

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

I've got no issue with the breaking change. My only gripe is with the active item styles.

@@ -228,18 +236,23 @@
background-color: #cacaca;
}

.active,
li:not(:last-of-type).active {
border: #0f62fe 1px solid;
Copy link
Owner

Choose a reason for hiding this comment

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

I'm not keen on using a border for the active state. It causes the text of an active item to shift. I'd prefer keeping the existing style of using a background color to distinguish the active but not selected state.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great, what about this blue: #d8e9f3
image

@Amerlander
Copy link
Contributor Author

I just also pushed a change which prevents disabled items from beeing select by keyboard or as first entry.
Please have a look at before merging: 3ee7b6e

Copy link
Owner

@metonym metonym left a comment

Choose a reason for hiding this comment

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

What's the reason for extracting it into a function? I prefer keeping the logic in the markup

@Amerlander
Copy link
Contributor Author

its self-referencing, so I think it has to be a function. And its also called in afterUpdate() to handle if the first element is disabled.
So we would have the logic in two places if we keep it in the markup.

@Amerlander
Copy link
Contributor Author

Sorry, to less sleep last night, I missed updating the afterUpdate() function.
Is fixed now.

@metonym
Copy link
Owner

metonym commented Feb 21, 2021

A couple of things:

  • As much as possible, I prefer business logic triggered by events to be in the markup – even if it means duplication
  • I'm not sure if incrementing the index for a disabled item would fix the issue; you would need a while loop to find the next non-disabled item

@Amerlander
Copy link
Contributor Author

I increment the selectedIndex as long as the selectedItem turns out to be disabled. To prevent infinite loop I stop after result.lenght-times. In my test it actually is fixing the issue. I just was mentally not able to bring it to github in the first try.

I had different approaches now, first is the function way of this pr, tested and working

function setNextSelectedIndex(i = 0) {
    i += 1; // index for stop the itteration
    selectedIndex += 1;
    if (selectedIndex === results.length) {
      selectedIndex = 0; // start over at the beginning of the array
    }
    if(results.lenght == 0 || !(selectedIndex in results) || i > results.lenght) {
      selectedIndex = -1; // no result found, gone trough the whole array
    } else if(results[selectedIndex].disabled) {
      setNextSelectedIndex(i); // no result found, not yet gone trough the whole array
    }
  };

In think an while loop would look something like that, but i havent tested yet

    selectedIndex += 1;
    if (selectedIndex === results.length) {
      selectedIndex = 0;
    }
    let i = 0;
    while(selectedIndex in results && results[selectedIndex].disabled && results.lenght > i) {
        i += 1;
        selectedIndex += 1;
        if (selectedIndex === results.length) {
          selectedIndex = 0;
        }
    }
    if(results[selectedIndex].disabled) {
      selectedIndex = -1;
    }

There was one approach with a lot of .filter()-stuff I gave up on.

I found the for working pretty nice and will put this into this pr.
Incrementation:

    for (selectedIndex++;(selectedIndex in results && results[selectedIndex].disabled); selectedIndex++) { }
    if(!(selectedIndex in results) || (selectedIndex in results && results[selectedIndex].disabled)) {
        selectedIndex = results.findIndex(result => !result.disabled);
    }

for the decrementation works similar:

    for (selectedIndex--;(selectedIndex in results && results[selectedIndex].disabled); selectedIndex--) { }
    if(!(selectedIndex in results) || (selectedIndex in results && results[selectedIndex].disabled)) {
        let reverseselectedIndex = results.slice().reverse().findIndex(result => !result.disabled) + 1;
        selectedIndex = (reverseselectedIndex == -1) ? -1 : (results.length - reverseselectedIndex);
    }

In the afterUpdate() function we can just find the first selectable item by results.findIndex(result => !result.disabled);, so there even is no repeating code.

@Amerlander
Copy link
Contributor Author

Amerlander commented Feb 21, 2021

it feels a bit weird to have a for-loop without an acutal function.

So I don't know whether to write for(); or for() {}.

@metonym
Copy link
Owner

metonym commented Feb 21, 2021

A couple of requirements:

  • when all items are disabled, I get stuck in an infinite loop when using the arrow keys
  • findIndex is not supported in IE

@metonym
Copy link
Owner

metonym commented Feb 21, 2021

Surely there's a more "Svelte" way of doing this.

Just thinking a loud here. Say that you have a reactive assignment that only tracks non-disabled items:

$: enabledResults = results.filter(item => !item.disabled);

Then when incrementing/decrementing using arrow keys, you could use enabledResults instead of results to determine to pivot the selectedIndex.

@Amerlander
Copy link
Contributor Author

Amerlander commented Feb 22, 2021

Ok, I'll try again. Thanks for your patience, I`m pretty new to svelte.

My first approach still uses .findIndex().
Is IE in Svelte an issue? I`m not that much into it and hope when the support for IE stops in 6 Months IE will not be an issue anywhere else too, but I thought Svelte is in its standard configuration not compatible to IE at all.
Regarding this I read this blog article: https://blog.az.sg/posts/svelte-and-ie11/
So do we have to keep IE in mind while developing svelte components, or is this handled by polyfills anyways?

  $: enabledResults = results.filter(item => !item.disabled);
         case 'ArrowDown':
          e.preventDefault();
          enabledIndex = enabledResults.findIndex(item => item == results[selectedIndex]);
          if(enabledIndex == -1) break;
          enabledIndex += 1;
          if (enabledIndex === enabledResults.length) {
            enabledIndex = 0;
          }
          selectedIndex = results.findIndex(item => item == enabledResults[enabledIndex]);
          break;
        case 'ArrowUp':
          e.preventDefault();
          enabledIndex = enabledResults.findIndex(item => item == results[selectedIndex]);
          if(enabledIndex == -1) break;
          enabledIndex -= 1;
          if (enabledIndex < 0) {
            enabledIndex = enabledResults.length - 1;
          }
          selectedIndex = results.findIndex(item => item == enabledResults[enabledIndex]);
          break;

@Amerlander
Copy link
Contributor Author

Amerlander commented Feb 23, 2021

I'll go with findIndex, since IE already need at least one polyfill for promise.
I wrote a note in the readme. If you find something what doesn't need a polyfill I'll take this of course.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants