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

It'd be nice to be able to shift-click a selection of a large CheckboxGroup #4

Open
BerendPronk opened this issue Oct 7, 2019 · 3 comments
Assignees
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@BerendPronk
Copy link

Title is self explanatory

@AlexLisenkov AlexLisenkov added enhancement New feature or request help wanted Extra attention is needed labels Nov 21, 2019
@AlexLisenkov AlexLisenkov removed the help wanted Extra attention is needed label Dec 11, 2019
@AlexLisenkov AlexLisenkov self-assigned this Dec 11, 2019
@AlexLisenkov
Copy link
Member

AlexLisenkov commented Dec 11, 2019

Not a good description, but thanks for the suggestion.

I will be working on a shift-click feature, but want feedback on my solution.
There are some issues that make this hard to develop, currently there is no order in how checkboxes are stored in the map.

POC branch: shift-click
CodeSandbox example: https://codesandbox.io/s/tender-tharp-tnysj

As a proof of concept I created a function that loops through the children of the CheckboxGroup.

const getChildCheckboxes = (childNodes: ReactNode[]): string[] => {
  const checkboxList: string[] = [];

  if (!Array.isArray(childNodes)){
    childNodes = [childNodes];
  }

  childNodes.forEach((node => {
    if (objectIsCheckbox(node)) {
      checkboxList.push(node.props.id);
    } else if (objectHasChildren(node)) {
      checkboxList.push(
        ...getChildCheckboxes(node.props.children)
      );
    }
  }));

  return checkboxList;
};

getChildCheckboxes(children as ReactNode[] || [])

To (un)check the checkboxes within the range I created this function, where lastToggledId is the previously toggled checkbox.

const toggleShiftGroup = (key: string): void => {
    const endCheckbox = checkboxes.get(key);

    if (!endCheckbox || lastToggledId === undefined) {
      return;
    }

    const start = orderedIdList.indexOf(lastToggledId);
    const end = orderedIdList.indexOf(key);
    const toggleCheckboxes = orderedIdList.slice(start < end ? start : end, (end > start ? end : start) + 1);

    toggleCheckboxes.forEach((id) => {
      const toggleCheckbox = checkboxes.get(id);
      if (toggleCheckbox) {
        toggleCheckbox.setIsChecked(endCheckbox.isChecked || false);
      }
    });

    onCheckboxChange(key);
  };

It seems to be working but I'm not sure about performance and should be tested carefully.

@AlexLisenkov AlexLisenkov added the help wanted Extra attention is needed label Dec 11, 2019
@brokenhappy
Copy link

brokenhappy commented Dec 12, 2019

Well if Im only commenting on the readability of the logic, the getChildCheckboxes could be refactored to use flatMap

const getChildCheckboxes = (childNodes: ReactNode[]): string[] => {
  if (!Array.isArray(childNodes)){
    childNodes = [childNodes];
  }

  return childNodes.flatMap(node => {
    if (objectIsCheckbox(node)) {
      return [node.props.id];
    } if (objectHasChildren(node)) {
      return getChildCheckboxes(node.props.children);
    }
  });
};

And the orderedIdList.slice() could be refactored to use Math.min and Math.max statement

const toggleCheckboxes = orderedIdList.slice(Math.min(start, end), Math.max(end, start) + 1);

@AlexLisenkov
Copy link
Member

Thanks for the suggestion.
Unfortunately after some testing I found out that iterating though children prop does not work when a subcomponent renders a Checkbox that is not part of the children prop (should've guesses that).

I am thinking about another solution, maybe iterate though the dom tree.
Could trigger the algo when the checkbox map changes (debounced), reindexing the ordered array.
Not sure how to decide the implementation of objectIsCheckbox yet, could just check if the node is a checkbox containing an id.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants