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

Add No Results Row #272

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

type1fool
Copy link

Description

When a Flop table has no rows in the table body, there is no messaging about empty contents. Although, :no_results_content is listed as an option, it appears to have no impact on the UI.

This PR adds a :no_results_attrs option and a new <tr> that is rendered when there are no @items. This solution works with both lists and LiveStreams.

Checklist

  • I added tests that cover my proposed changes.
  • I updated the documentation related to my changes (if applicable).

image

image

@type1fool type1fool changed the title add :no_results_attrs opt, add empty row Add No Results Row Oct 13, 2023
@type1fool
Copy link
Author

@woylie This is a follow up from the previous discussion. I was reminded that CSS pseudo selectors can be used for this type of scenario, where we can't check a LiveStream for emptiness.

Hat tip to @clarkware. 🤠

@type1fool type1fool marked this pull request as ready for review October 16, 2023 14:10
@woylie woylie self-requested a review October 17, 2023 05:08
@woylie
Copy link
Owner

woylie commented Oct 18, 2023

Thanks for opening this PR. I have a few thoughts to consider.

  1. This library generally adheres to using semantic class names, and only:table-row is a Tailwind class.
  2. From a semantic perspective, a paragraph may be a more fitting choice than a table with headers and a single cell in the body.
  3. Using a paragraph would likely offer a better experience for users who depend on screen readers.

If we decide to stick with the original approach instead, encapsulating the condition in a named function could improve clarity:

defp no_results?([]), do: true
defp no_results?(%LiveStream{inserts: []}), do: true
defp no_results?(_), do: false

However, the LiveStream struct is undocumented. I'm also not sure whether it is sufficient to just look at the inserts field.

An alternative could be adding an empty? attribute to the table component that defaults to false. The condition used in the component would then change to if @items == [] || @empty?. This allows the empty flag to be set on the socket based on query results, should you choose to use a stream.

@woylie woylie removed their request for review October 18, 2023 10:47
@type1fool
Copy link
Author

Thank you for the feedback, and sorry I didn't see your response earlier. There's a bit more complexity here than I expected, so please forgive the lengthy response below.

CSS Classes

I could set a style attr instead, but that would be disallowed when CSP is applied - unless the developer configures CSP correctly to allow inline styles. I ran into this issue when I enabled CSP on a new Phoenix project, where root.html.heex had a style attr on one of the elements iirc. Using a semantic name would follow the pattern you've set, but it would require each developer to know how to apply display properly if the row is the only child.

Conditional Rendering

My goal was to make this something the developer doesn't need to think about. I think conditionally rendering a <table> or <p> based on @items emptiness would be fine, but I'm also not sure how reliable LiveStream.inserts is for detecting emptiness. Adding an empty? attr wouldn't add much value in my opinion, since it would still require each developer to know how to detect emptiness, which is something they could already do.

While it's possible I had a wrong implementation, I believe I wasn't seeing the "No results" row when I used a similar approach that checked @items for an empty list or a stream with empty inserts.

Existing Patterns

Rendering a <p> instead of a <table> when there is no data may make sense, but the "No results" <tr> seems to be a well-established pattern. Searching "table with no data" on Google, the examples I see almost always render a table with a row conveying that there is no data. Maybe aria attributes could be used to clarify things for screen readers.

It's not obvious what to do next. I completely understand wanting to keep the components CSS framework-agnostic, but we'd need to figure out where to put custom styles if this is going to be something devs don't need to think about.

@woylie
Copy link
Owner

woylie commented Nov 11, 2023

Using a semantic name would follow the pattern you've set, but it would require each developer to know how to apply display properly if the row is the only child.

That's true, but the same is true for the Tailwind class if your project does not use Tailwind. It would be odd to have a class that's specific for one single framework. If we were to add this, I'd want to use a generic class name like is-only-child-visible or has-no-siblings-visible to be consistent. The necessary CSS could be added to the documentation, including a Tailwind example. This should probably be opt-in to prevent a breaking change.

Adding an empty? attr wouldn't add much value in my opinion, since it would still require each developer to know how to detect emptiness, which is something they could already do.

It depends on the situation, but if it's a paginated table where you replace the stream whenever you paginate, then you know whether there are results or not when you fetch the results. In that case, you could do something like:

def handle_params(params, _, socket) do
  {items, meta} = list_items(params)
  {:noreply, socket |> assign(:empty?, items == []) |> stream(items, replace: true)}
end

This doesn't work in an infinite scroll or more-button scenario where you keep adding items without removing the old ones, of course.

Maybe aria attributes could be used to clarify things for screen readers.

I don't think we can do anything with aria attributes about this. Here are some examples about how screen readers announce tables: https://a11y-101.com/development/tables. Screen readers would always see and announce the table. We cannot conditionally add aria attributes to part of the table, since it's a CSS based solution.

Side note, I just found that WCAG suggests to use role="status" for these kind of status messages (source). We should add that to the existing no-results message.

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