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

Semantics for saved search table #2697

Merged
merged 9 commits into from Jun 2, 2023

Conversation

stephanieleary
Copy link
Contributor

Adds scoped headers to saved search table for better screen reader support, as well as classes for CSS styling.

Adds scoped headers to saved search table for better screen reader
support, as well as classes for CSS styling.
Copy link
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

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

Thanks, @stephanieleary. I've just pushed up a couple of small changes here:

1.) I indented the HTML inside the new thead/tbody tags for consistency with other HTML styling.

2.) I added a 20% width to the time column in the LESS (moving it from being hard-coded in the template). Your previous changes removed this width setting, but I found that this caused the time column to be too wide when search terms were short, and too narrow when search terms were long; I believe that having an explicit width is still helpful for the visual appearance of the table.

Please let me know if you want to make changes or have any objections to these edits. If you're happy with the result, please take the PR out of draft mode, and then it can be merged.

I'm also going to ask @crhallberg for a review in case he has additional comments.

Copy link
Contributor

@crhallberg crhallberg left a comment

Choose a reason for hiding this comment

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

No further comments! Thank you very much!

@demiankatz
Copy link
Member

demiankatz commented Feb 21, 2023

@stephanieleary, I just merged the latest dev branch into this PR, as some conflicts were introduced by recent upstream changes. I believe this is ready to merge now, but just waiting for your thumbs-up.

@stephanieleary stephanieleary marked this pull request as ready for review February 21, 2023 16:44
@stephanieleary
Copy link
Contributor Author

Thanks for following up!

@demiankatz demiankatz added this to the 9.1 milestone May 2, 2023
@demiankatz
Copy link
Member

@stephanieleary, I think this may have gotten a bit lost in the shuffle. I've just merged the latest dev branch in and resolved conflicts. I think it's ready for a final round of testing before being merged.

Does @pasitiis have any suggestions or concerns?

Could @sturkel89 do a quick round of testing to compare this against the dev branch and make sure there are no significant visual anomalies? All changes here impact the search history screen, so it should be a matter of performing a few searches and saving some of them to fully populate the data displayed by the modified template. Turning on the schedule_searches setting in config.ini will also reveal some additional controls that are normally hidden.

@sturkel89
Copy link
Contributor

I've run some checks on the Search History screen in all three themes, using both the test branch and the dev branch.

I can't find any visible differences between test and dev in any theme, except for one very minor thing that I would say does not need to be fixed. The one difference is where the alternating row shading starts: in the test branch, it starts with the first item in the list of saved or recent searches. In the dev branch, the shading starts with the header row (Time, Search, Limits, Results, Alert Schedule, Delete).

Test branch - Sandal example:
image

Dev branch - Sandal example:
image

The same is seen in every theme. Looks good!

Copy link
Contributor

@EreMaijala EreMaijala left a comment

Choose a reason for hiding this comment

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

The 20% column width rule needs to be cleaned up. The rest is ok for me.

@demiankatz
Copy link
Member

Thanks, @EreMaijala -- I believe I've found a "best-of-both-worlds" approach to the LESS, so I've pushed up that improvement.

Regarding the color alternation, it's simply related to the fact that the alternation applies to td and not th, so converting the table header changes where the alternation begins. I imagine there's probably a way to make the alternation work across th and td, but that's beyond my area of expertise. I think we can live with it for now at least. :-)

@demiankatz demiankatz merged commit 0e8abd1 into vufind-org:dev Jun 2, 2023
7 checks passed
EreMaijala pushed a commit to EreMaijala/vufind that referenced this pull request Sep 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants