Skip to content

Conversation

@marcosvega91
Copy link
Member

@marcosvega91 marcosvega91 commented Jun 18, 2020

What:

  1. I have added mode as dependency of useEffect.
  2. I have added a new hook to sort the table.

Why:

  1. Because in this way the user could in future change the mode.
  2. Because the user could view logs better.

How:

  1. Updating the array of dep
  2. Adding a new hook to sort the table

I have also added two dependences

  1. react-hooks-testing-library
  2. @primer/octicons-react

I think we can replace other icons with the one of the library
Checklist:

  • Tests
  • Ready to be merged

@marcosvega91 marcosvega91 self-assigned this Jun 18, 2020
@marcosvega91 marcosvega91 added the bug Something isn't working label Jun 18, 2020
Copy link
Member

@smeijer smeijer left a comment

Choose a reason for hiding this comment

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

Can we also make the # column title clickable to change direction? That way this PR add's some functionality, in addition to adding the effect dependency. 😇

We can use the chevron-up and chevron-down icons to indicate direction.

@marcosvega91
Copy link
Member Author

Yes I can. I'll put the PR on draft until I've done

@marcosvega91 marcosvega91 marked this pull request as draft June 18, 2020 10:44
@marcosvega91 marcosvega91 changed the title fix: add mode as dependency in StickyList feat: add sorter for DomEvents Jun 18, 2020
@marcosvega91 marcosvega91 marked this pull request as ready for review June 18, 2020 15:00
@marcosvega91 marcosvega91 added feature New feature or request and removed bug Something isn't working labels Jun 18, 2020
Copy link
Member

@smeijer smeijer left a comment

Choose a reason for hiding this comment

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

Thanks! This is really awesome!

I do think we can simplify it a bit though. In the way I look at this, it doesn't make sense to add sorting to any other column than the id. The event log is meant to see which events a certain action trigger.

Changing the order of events (click -> change -> click to click -> click -> change) , renders the data useless.

So I believe we should only support sorting by id.

If we only sort on ID, we can also simplify this code. We don't need a "sorting" hook. We just need to call buffer.current.reverse() and flip between buffer.current.splice(0, 0, log) and buffer.current.push(log).

What do you think?

@marcosvega91 marcosvega91 requested a review from smeijer June 19, 2020 08:48
@smeijer smeijer merged commit 285f355 into testing-library:develop Jun 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants