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

fix: CvDataTable did not properly store/emit sort events #1543

Conversation

markusdobler
Copy link
Contributor

What was broken?

Clicking on a sortable header in a CvDataTable (e.g. the first two columns in the official story book didn't actually trigger the expected events:

  • the sort icon for that column remained 'unsorted'
  • the action always has order: undefined

Why ?

The code that emits cv:sort when clicking on a CvDataTableHeading uses the key value:

function onSortClick() {
  bus?.emit('cv:sort', {
    heading: { id: cvId.value, name: props.name },
    value: nextOrder[internalOrder.value],
  });
}

However, the receiving code destructs payload into a variable named val:

function onSort(payload) {
  const { heading, val } = payload;

As val doesn't acutally exist in the payload, the variable gets initialized to undefined, breaking the rest of that function.

What did you do?

Changed the code in onSort(payload) to use the variable named value instead of val.

How have you tested it?

  • built the storybook with the fixed code
  • opened the story for CvDataTables
  • clicked on the sortable header
  • confirmed that the sort indicator now iterates through "ascending", "descending" and "none"
  • confirmed that the emitted onSort action now has the proper order

Were docs updated if needed?

  • N/A
  • No
  • Yes

Copy link
Collaborator

@davidnixon davidnixon left a comment

Choose a reason for hiding this comment

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

LGTM

@davidnixon
Copy link
Collaborator

Thanks for the fix!

@kodiakhq kodiakhq bot merged commit 15b0eb3 into carbon-design-system:main Oct 19, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants