imballinst / react-bs-datatable

Bootstrap datatable without jQuery. Features include: filter, sort, pagination, checkbox, and control customization.
https://imballinst.github.io/react-bs-datatable
MIT License
59 stars 20 forks source link

Trigger Sort From Outside #123

Closed ahmetunal closed 2 years ago

ahmetunal commented 2 years ago

Need: Trigger sort functionality from outside of the component Changing initialState of the sortProps after initial render wasn't resorting, so I added a useEffect on sortProps change to force re-sort.

@imballinst

imballinst commented 2 years ago

@ahmetunal hmmm, is your intention uncontrolled table or controlled table? The initialState is only meant for uncontrolled tables. An uncontrolled table is something like: you give an initial state to the table, then let the table handle the rest (whatever you give to the initialState, it's only used in the initialization of the state).

https://github.com/imballinst/react-bs-datatable/blob/32e86e3050211f490cc46d226525bab526d4ea65/src/components/DatatableWrapper.tsx#L207-L216

If you want to have a controlled table, then I guess you need to create functions outside of the table, something like this:

https://github.com/imballinst/react-bs-datatable/blob/32e86e3050211f490cc46d226525bab526d4ea65/src/__stories__/01-Controlled.stories.tsx#L98-L125

ahmetunal commented 2 years ago

@imballinst I need it on controlled table, even if the table takes care of its sorting itself, still I need outside influence on sorting, I mean sort the table onClick of another component

imballinst commented 2 years ago

@ahmetunal Yes, in that case, I think controlled table it is (?) When we are using controlled table, the "internal filter/sort/pagination" will not affect anything. The only states used are the ones passed from controlledProps.

If you use both controlledProps and initialState, the initialState will not matter because the components will prioritize the controlled states (which is passed from outside DatatableWrapper):

https://github.com/imballinst/react-bs-datatable/blob/32e86e3050211f490cc46d226525bab526d4ea65/src/components/TableHeader.tsx#L75-L81

Actually, I think I need a concrete use case. Could you create a sandbox to simulate your issue?

ahmetunal commented 2 years ago

@imballinst Here is a use case: https://codesandbox.io/s/react-bs-datatable-outside-sort-3g3f78

ahmetunal commented 2 years ago

Oh wait, I think I can use controlledProps->sortState actually , right?

imballinst commented 2 years ago

Oh wait, I think I can use controlledProps->sortState actually , right?

sorry for the late reply, yes, indeed! :100: Something like this:

https://github.com/imballinst/react-bs-datatable/blob/32e86e3050211f490cc46d226525bab526d4ea65/src/__stories__/01-Controlled.stories.tsx#L182-L188

ahmetunal commented 2 years ago

@imballinst I don't think it works properly, and disrupts datatable's own behavior I updated the sandbox: https://codesandbox.io/s/react-bs-datatable-outside-sort-3g3f78

To test:

Am I doing something wrong here?

imballinst commented 2 years ago

@ahmetunal ah, yes, you are correct. I could replicate the issue in the sandbox, as well as in the Storybook, my bad! https://imballinst.github.io/react-bs-datatable/?path=/story/controlled--async

I'll try to fix it today + add more tests that can cover the controlled parts properly.

ahmetunal commented 2 years ago

@imballinst awesome, thanks

imballinst commented 2 years ago

@ahmetunal I just published v3.0.2-alpha.0 which can be tested in this sandbox: https://codesandbox.io/s/react-bs-datatable-outside-sort-forked-ue79kn?file=/src/App.js

https://user-images.githubusercontent.com/7077157/155458729-3becdac3-60ae-4515-ab8a-bb7ff5f966b2.mov

It would seem now that it works, could you confirm? If it is confirmed, then I will publish v3.0.2 after. Thanks!

ahmetunal commented 2 years ago

@imballinst I tried the sandbox, it works as expected. From what I understand from the code, you are doing the sorting outside of the component with helper functions, and passing it to the component as new data, not inside the component. Is this the intended way?

imballinst commented 2 years ago

@ahmetunal I'm afraid that's the likeliest way right now. Alternatively, I can try experimenting with useImperativeHandle https://reactjs.org/docs/hooks-reference.html#useimperativehandle. So, it'll be something like this:

// DatatableWrapper
useImperativeHandle(tableEventsRef, () => ({
  sort: (nextSort: SortType) => {
    onSortChange(nextSort);
  }
}));

// App.tsx
const tableEventsRef = useRef<TableEventsRef>()

<button onClick={() => tableEventsRef.sort("name")}>Test</button>

<DatatableWrapper tableEventsRef={tableEventsRef}>
  // ...
</DatatableWrapper>

What do you think about that?

imballinst commented 2 years ago

@ahmetunal I have created 3.1.0-alpha.0 (minor version bump because it's a new feature + the onSortChange API changes): https://codesandbox.io/s/react-bs-datatable-3-1-0-alpha-0-qlreym?file=/src/App.js

We could now trigger events (not only sort, but also filters, etc.) from outside the uncontrolled table, something like this:

export default function App() {
  const tableEventsRef = useRef();

  return (
    <>
      <button onClick={() => tableEventsRef.current?.onSortChange("name")}>
        Sort by name
      </button>
      <button onClick={() => tableEventsRef.current?.onSortChange("score")}>
        Sort by score
      </button>

      <DatatableWrapper
        body={TABLE_BODY}
        headers={STORY_HEADERS}
        tableEventsRef={tableEventsRef}
        paginationOptionsProps={{
          initialState: {
            rowsPerPage: 10,
            options: [5, 10, 15, 20]
          }
        }}
      >
        <Row className="mb-4 p-2">
          <Col
            xs={12}
            lg={4}
            className="d-flex flex-col justify-content-end align-items-end"
          >
            <Filter />
          </Col>
          <Col
            xs={12}
            sm={6}
            lg={4}
            className="d-flex flex-col justify-content-lg-center align-items-center justify-content-sm-start mb-2 mb-sm-0"
          >
            <PaginationOptions />
          </Col>
          <Col
            xs={12}
            sm={6}
            lg={4}
            className="d-flex flex-col justify-content-end align-items-end"
          >
            <Pagination />
          </Col>
        </Row>
        <Table>
          <TableHeader />
          <TableBody />
        </Table>
      </DatatableWrapper>
    </>
  );
}

Let me know what you think!

ahmetunal commented 2 years ago

Thanks @imballinst, I'll try this and let you know

imballinst commented 2 years ago

@ahmetunal I have another idea, what if, there is this function useControlledProps, which we can use like this?

function App() {
  const controlledProps = useControlledProps(headers, body, {
    rowsPerPageOptions: [5, 10, 15, 20]
  });

  return (
    <>
      <div style={{ padding: "10px", background: "#efefef" }}>
        <label>Sort By: </label>
        <button onClick={() => controlledProps.tableHeaderProps.onSortChange(getNextSort(sortState, "name"))}>
          Name
        </button>
        <button
          onClick={() => controlledProps.tableHeaderProps.onSortChange(getNextSort(sortState, "username"))}
        >
          Username
        </button>
        <button
          onClick={() => controlledProps.tableHeaderProps.onSortChange(getNextSort(sortState, "location"))}
        >
          Location
        </button>
        <button onClick={() => controlledProps.tableHeaderProps.onSortChange(getNextSort(sortState, "score"))}>
          Score
        </button>
      </div>
      <DatatableWrapper body={data} headers={headers} isControlled>
        <Row className="mb-4">
          <Col
            xs={12}
            lg={4}
            className="d-flex flex-col justify-content-end align-items-end"
          >
            <Filter controlledProps={controlledProps.filterProps} />
          </Col>
          <Col
            xs={12}
            sm={6}
            lg={4}
            className="d-flex flex-col justify-content-lg-center align-items-center justify-content-sm-start mb-2 mb-sm-0"
          >
            <PaginationOptions
              controlledProps={controlledProps.paginationOptionsProps}
            />
          </Col>
          <Col
            xs={12}
            sm={6}
            lg={4}
            className="d-flex flex-col justify-content-end align-items-end"
          >
            <Pagination
              controlledProps={controlledProps.paginationProps}
            />
          </Col>
        </Row>
        <Table>
          <TableHeader
            controlledProps={controlledProps.tableHeaderProps}
          />
          <TableBody
            controlledProps={controlledProps.tableBodyProps}
          />
        </Table>
      </DatatableWrapper>
    </>
  );
}

So, it's still controlled, but all the variables are "premade" so we don't need to declare them one-by-one. We can also "call" the, for example, onSortChange this way. The benefits of this approach:

  1. Prevent using an anti-pattern, as quoted in https://reactjs.org/docs/hooks-reference.html#useimperativehandle:

As always, imperative code using refs should be avoided in most cases

  1. No breaking API changes
  2. A new function to easily bootstrap the controlled states

Let me know what you think! I think personally it's better, but it might be just from me.

ahmetunal commented 2 years ago

@imballinst

imballinst commented 2 years ago

@ahmetunal got it! Let me see what I can do, then I'll get back to this PR after I have finished implementing it.

imballinst commented 2 years ago

@ahmetunal after considering things, I think I'll stick to the tableEventsRef for now. If I want to use the alternative approach, I need to make a lot of breaking things and I don't think I want to do that right now (because the API would seem to be "unstable"). There is 1 change, so we are using onSortByPropChange instead of onSortChange, so something like this:

<div>
  <button
    onClick={() => tableEventsRef.current?.onSortByPropChange('name')}
  >
    External sort by name
  </button>
  <button
    onClick={() => tableEventsRef.current?.onSortByPropChange('username')}
  >
    External sort by username
  </button>
</div>

Otherwise, the previous onSortChange API will change and it will break things (which I don't want).

I'll merge https://github.com/imballinst/react-bs-datatable/pull/126, then I'll publish 3.1.0.

imballinst commented 2 years ago

The Storybook demo for this use case: https://imballinst.github.io/react-bs-datatable/?path=/story/uncontrolled--uncontrolled-with-ref-events

ahmetunal commented 2 years ago

@imballinst, sorry for the late reply. I pulled the v3.1.0 and it works great, thanks for the great work. I believe we can close this pr now.

imballinst commented 2 years ago

@imballinst, sorry for the late reply. I pulled the v3.1.0 and it works great, thanks for the great work. I believe we can close this pr now.

No worries. Thanks a lot for the feedback and helping in the iteration! I'm closing this PR.