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

Use list items for spelling suggestions #3645

Merged
merged 4 commits into from
May 8, 2024

Conversation

maccabeelevine
Copy link
Member

Follow-up to #3640 to use an actual list for the "list" of suggestions, for semantics / accessibility.

@@ -77,10 +77,10 @@ public function renderSpellingSuggestions($msg, $results, $view)
}

$html = '<div class="' . $this->getContainerClass() . '">';
$html .= '<h2 class="spellingSuggestions">' . $msg . '</h2>';
$html .= '<h2 class="spellingSuggestions">' . $msg . '</h2><ul>';
Copy link
Member Author

Choose a reason for hiding this comment

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

On second thought, maybe this should be an ordered <ol> since it matches the order of the search terms?

Copy link
Member

Choose a reason for hiding this comment

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

The other question is whether the inner list of suggestions should also be a list! Because we have an outer list of input terms, and then each input term has its own independent list of suggestions. I think you could argue for ordered or unordered lists in each case as well. There is an order to both (input order for the input list, and relevance order for the output list) but it's not especially obvious and may not mean much to the end user.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree -- the inner list should be a list as well.

I think I'm going to skip the ordered lists, although it's correct in some sense, it won't be obvious to the user.

@maccabeelevine maccabeelevine marked this pull request as ready for review May 7, 2024 21:00
@demiankatz
Copy link
Member

Thanks, @maccabeelevine, this looks great to me, and the markup is definitely cleaner than before. I agree that adding the bullets makes sense. However, before I merge this, I'd like to get a second opinion.

@sturkel89, do you mind checking this in all three themes and sharing an opinion? The spelling suggestions should look the same in both places, except this branch makes them a bulleted list.

To easily see spelling results, I suggest:

1.) In config.ini, comment out the uncommented dictionaries[] settings, and uncomment dictionaries[] = direct
2.) Do a search for tset OR hmtl

That should give you a couple of things to look at.

@demiankatz demiankatz requested a review from sturkel89 May 8, 2024 11:06
Copy link
Contributor

@sturkel89 sturkel89 left a comment

Choose a reason for hiding this comment

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

I reviewed the test branch in all three themes, and compared it to dev.

The bullets make it easier to see the terms for which there are spelling suggestions, and I definitely think an unordered list makes more sense than an ordered list.

The feature is pretty much identical in the three themes, so I'm only providing screenshots from sandal:

Test:

image

image

Dev:

image

image

@demiankatz demiankatz added this to the 10.0 milestone May 8, 2024
@demiankatz demiankatz merged commit aebe2b7 into vufind-org:dev May 8, 2024
7 checks passed
@maccabeelevine maccabeelevine deleted the spelling-suggestions-list branch May 8, 2024 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants