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

Improve positioning of sort indicator #833

Closed
wants to merge 1 commit into from
Closed

Improve positioning of sort indicator #833

wants to merge 1 commit into from

Conversation

gwww
Copy link

@gwww gwww commented Jan 16, 2023

Before submitting the PR:

  • Does your PR reference an issue? If not, please chat to the team on Discord or GitHub before submission.
  • Did you update and run tests before submission using npm run test?
  • Does your branch follow our naming convention? If not, please amend the branch name using branch -m new-branch-name
  • Did you update documentation related to your new feature or changes?

What does your PR address?

When the sort header of a Data Table is clicked the columns shift because of the addition/removal of the table-sort-asc or table-sort-dsc class. Adding position absolute prevents the class change affecting column width.

@vercel
Copy link

vercel bot commented Jan 16, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
skeleton-docs ❌ Failed (Inspect) Jan 16, 2023 at 1:19AM (UTC)

@gwww
Copy link
Author

gwww commented Jan 16, 2023

Well, not my best first PR! In more testing the absolute worked... just it was in both the x and y axis. I'll put this as draft for now and see if I can amp up my CSS guru from within.

@gwww gwww marked this pull request as draft January 16, 2023 20:06
@endigo9740
Copy link
Contributor

@gwww This is on my list to review - just a very busy week so it may take some time.

@gwww
Copy link
Author

gwww commented Jan 17, 2023

Thx Chris. It doesn't work as it is. I've been looking for other solutions. Most seem to be more complex and require both code and possibly markup changes to work. When I've found something I move this out of draft. And, I don't want to distract you!

@endigo9740
Copy link
Contributor

endigo9740 commented Jan 17, 2023

I'm actually working through all the Tailwind styled elements in the library now to clean up and refine things. If all else fails I can probably adjust this as I go:

@gwww
Copy link
Author

gwww commented Jan 17, 2023

Here is what I ended up doing in my code to get it working such that the sort indicators don't cause the column width to change. It took much longer than I hoped as I believe I ran into a SvelteKit bug (which is next on my list to confirm) where the attribute specific CSS classes were optimized away (the last two classes, below). I ended up moving the CSS to a .css file to fix.

My repo is here if you want to look at the table I built. I can put the CSS/TS fix into a PR if you wish but figure I would let you do whatever refactoring you are doing. I also recognize the DataTable is post 1.0, so also feel free to just close this and track it through #538.

CSS:

/* Styles placed in separate file as SvelteKit bug optimizes out the last two classes */
/* TODO: convert to TailwindCSS */
.table-asc::after {
  content: '↓';
  margin-left: 0.5rem;
  visibility: hidden;
  opacity: 50%;
}
.table-dsc::after {
  content: '↑';
  margin-left: 0.5rem;
  visibility: hidden;
  opacity: 50%;
}
.table-asc[aria-sort='ascending']::after {
  visibility: visible;
}
.table-dsc[aria-sort='descending']::after {
  visibility: visible;
}

TS; my replacement for the DataTable on:click handler:

  function handleSortEvent(e: Event) {
    if (!(e.target instanceof Element)) return;
    const sortTarget = e.target as HTMLElement;
    const node = sortTarget.closest('thead')!;

    // Set desired new sort order
    const sortAscending = sortTarget.getAttribute('aria-sort') !== 'ascending';
    const sortColumn = sortTarget.getAttribute('data-sort');
    if (!sortColumn) return;

    node.querySelectorAll<HTMLElement>(`.${classAsc}, .${classDsc}`).forEach((el) => {
      el.setAttribute('aria-sort', 'none');
    });

    sortTarget.classList.remove(classAsc);
    sortTarget.classList.remove(classDsc);
    sortTarget.classList.add(sortAscending ? classAsc : classDsc);
    sortTarget.setAttribute('aria-sort', sortAscending ? 'ascending' : 'descending');

    sortTable(sortTarget.closest('table')!, Number(sortColumn), sortAscending);
  }

@endigo9740 endigo9740 mentioned this pull request Jan 18, 2023
54 tasks
@endigo9740
Copy link
Contributor

endigo9740 commented Jan 19, 2023

I'm going to close out this PR as a fix has been implemented as part of:

See the tables.css section - I've included a note related to this. Thanks!

@endigo9740 endigo9740 closed this Jan 19, 2023
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.

2 participants