Turn my account menu into ul, because it is a list

[ckaz]

This turns my account menu into an ul, because it is a list semantically. Facets sidebar will follow in separate PR.

TODO:

• Merge Corrects SCSS compile extend error #2922 to dev (and dev to here) before continuing work.

[demiankatz]

Thanks, @ckaz! This makes sense to me. I feel like this may overlap with some work in progress in other open PRs, but I think it would be great to take care of this one detail first, and we can reconcile any conflicts after that.

@sturkel89, do you mind giving this a visual inspection in all the themes (comparing against dev) and let us know if you spot any anomalies? The code changes affect the right sidebar menu in the account screen -- the account options, and the favorite lists area. As a test, I'd suggest turning on the Demo driver and creating a couple of named favorite lists. That should populate all the menu options that are impacted by these code changes.

[sturkel89]

I've inspected the test branch in all three themes and compared the output with the dev branch. All three themes behaved identically; there was no variation between the themes.

My main comments relate to aesthetics:

• The padding between the bullets and the items is a bit wide for my taste, leading to increased word wrap in those items. Could the bullets align at the far left margin of their areas, under the Y in Your Account and in Your Lists? If so, then the bulleted items could remain in the same relative position under the headings as they are in the dev branch (with less word wrapping).

• I note that the bullets seem to be center-aligned with the items, rather than top-aligned. I think it is more standard to have bullets align with the top line of a multi-line bulleted text block.

• Finally, the number indicating how many items are on a saved list appears in a stretchable dot rather than a fixed-size dot that fits the contents (4, in these cases). The square "dots" around the number of checked out items and storage retrieval requests in the Your Account area do not stretch; rather, they align with the top line of the text block. I think the numbers for the saved items should have that same behavior.

Example from test branch:

Same content in dev branch:

[demiankatz]

Thanks, @sturkel89. I don't think any of those bullets are supposed to be visible at all -- I believe the intent is that this branch should look exactly like the dev branch, which it clearly does not.

@ckaz, I think you need to run npm run css and commit the resulting changes to compiled.css.

@sturkel89, I agree that those stretched-out list numbers are definitely not supposed to look like that. Is that happening in the dev branch as well, or only in this PR? If it's happening in dev, we should look into fixing that for the next minor release.

[EreMaijala]

@pasitiis Could you check this at least wrt accessibility? Comparing to our local version we have role="none" for the list items and role="menuitem" for the actual links (and of course role="menu" for the container).

[ckaz]

@ckaz, I think you need to run npm run css and commit the resulting changes to compiled.css.

@demiankatz Sorry, my oversight, since we never push our CSS files here. However, my compiled CSS looks very differently from yours -- any tricks you apply? And: How do you run lessToSass in the current dev version?

My main comments relate to aesthetics:

@sturkel89 Indeed, there shouldn't be any list bullets. I'll look into the drawn out badge number issue.

[ckaz]

I agree that those stretched-out list numbers are definitely not supposed to look like that. Is that happening in the dev branch as well, or only in this PR? If it's happening in dev, we should look into fixing that for the next minor release.

@demiankatz It is in the dev branch already. When a list item has a title running over more than one line, the badge gets stretched. Maybe a max-height could sort this out.

[demiankatz]

I agree that those stretched-out list numbers are definitely not supposed to look like that. Is that happening in the dev branch as well, or only in this PR? If it's happening in dev, we should look into fixing that for the next minor release.

@demiankatz It is in the dev branch already. When a list item has a title running over more than one line, the badge gets stretched. Maybe a max-height could sort this out.

Thanks for confirming, @ckaz. I see that it is also an issue in the release-9.0 branch, so I'll see if I can find a fix that we can incorporate into release 9.0.2. Since the exact same badge works fine in the facet display, there's probably a minor inconsistency that can be corrected. I'll see if I can figure it out, and if not, I'll recruit @crhallberg to help.

UPDATE: see #2916 for work in progress on a solution.

[demiankatz]

@demiankatz Sorry, my oversight, since we never push our CSS files here. However, my compiled CSS looks very differently from yours -- any tricks you apply? And: How do you run lessToSass in the current dev version?

@ckaz, it looks like it got compiled in dev mode instead of production mode, so it's not minified and decommented. Probably my fault because I gave you the wrong command to run -- should be npm run build:css not just npm run css. My apologies; still trying to form new habits myself. (Note that the old grunt commands should still work for now, but I'm trying to get used to npm run for when we eventually phase grunt out).

For lessToSass, you can currently do either npm run lessToSass or grunt lessToSass and they should have the same effect.

[demiankatz]

Thanks, @ckaz, this is looking better now... but I see there are some conflicts that need to be resolved with dev. Might as well wait until #2916 is fully resolved and we hear back from @pasitiis regarding @EreMaijala's question, and then we can merge everything together and do another round of tests on this.

[ckaz]

@ckaz, it looks like it got compiled in dev mode instead of production mode, so it's not minified and decommented. Probably my fault because I gave you the wrong command to run -- should be npm run build:css not just npm run css.

@demiankatz Yes, I had figured that much from your post in @elsenhans PR, but weirdly enough, I got errors from npm trying to install node-sass 9.0.0 -- I pinned it to use version 8 and then things started to work. It still doesn't look super-safe yet. Running
npm run build:css
I get

jit-grunt: Plugin for the "css" task not found.
If you have installed the plugin already, please setting the static mapping.
See https://github.com/shootaroo/jit-grunt#static-mappings

Warning: Task "css" failed. Use --force to continue.
Aborted due to warnings.

npm run watch:css seems to work well

npm run build:scss fails due to errors with a nested selector extend

[demiankatz]

@ckaz, did you run npm install in the VuFind directory prior to trying other commands? I wonder if that could be a factor here. It's also possible that things work inconsistently in newer Node versions, since VuFind's configuration hasn't changed much in years and may be geared toward older versions. I'm happy to compare notes on versions if that would help.

[ckaz]

@pasitiis Could you check this at least wrt accessibility? Comparing to our local version we have role="none" for the list items and role="menuitem" for the actual links (and of course role="menu" for the container).

@pasitiis , @EreMaijala -- this sounds like a reasonable addition!

[ckaz]

@ckaz, did you run npm install in the VuFind directory prior to trying other commands?

Yes, I did. I even chucked out the entire directory when I couldn't pin down the error. I'll keep an eye on it.

[demiankatz]

@ckaz, I checked out your branch and ran npm run build:css and it introduced changes to two of the compiled.css files, but not all of them. Very strange. In any case, I committed those changes, and then went ahead and merged in the latest dev branch to clean out the conflicts while I was in the area anyway.

I'd be interested to know, if you check this out and run npm run build:css yourself, if it introduces changes or keeps things the same.

[demiankatz]

Also, I'm seeing the same error you are when I run npm run build:scss -- and it happens both here and in the dev branch, so it's not directly related to this PR. Maybe @crhallberg can offer a theory about this.

[ckaz]

@ckaz, I checked out your branch and ran npm run build:css and it introduced changes to two of the compiled.css files, but not all of them. Very strange. In any case, I committed those changes, and then went ahead and merged in the latest dev branch to clean out the conflicts while I was in the area anyway.

I'd be interested to know, if you check this out and run npm run build:css yourself, if it introduces changes or keeps things the same.

@demiankatz I just checked it out and ran npm run build:css and I get

> vufind@9.0.1 build:css
> npm run build:less


> vufind@9.0.1 build:less
> grunt less && npm run lessToSass

sh: Zeile 1: grunt: Kommando nicht gefunden.

The last line translates into "sh: Line 1: grunt: command not found"

However, it wrote CSS files to all themes (bootprint3, bootstrap3, sandal, local_theme_example).
Git history references as latest change my last commit 7e70d32 of 13:59 yesterday ...

[demiankatz]

@ckaz, I think perhaps with regard to grunt, you have the tool installed in the local NPM directory of your VuFind checkout, but not globally -- so it works when it is invoked through npm run (which includes the local directory in its search path) but it doesn't work when you run it directly from the command line (because it's not in your system search path).

I'm still baffled why your local run would create some files identically to mine, but others differently. Weird!

[demiankatz]

Thanks, @ckaz! I wonder if we should add an SCSS compile step to the GitHub Actions process, so if something breaks compatibility it will also break the build and we can sort it out right away.

[demiankatz]

I've added a comment to #2922 and opened #2924 to help us prevent this situation from occurring again in future.

[stephanieleary]

Re: the question of roles, the menu role should not be used here. Adrian Roselli has written an excellent article explaining the drawbacks.

I think these lists should be enclosed in nav tags with aria-labelledby attributes that point to the list titles ("Your Account," "Your Lists," etc.) so they have unique names in the screen reader's landmarks and headings list.

[pasitiis]

@pasitiis Could you check this at least wrt accessibility? Comparing to our local version we have role="none" for the list items and role="menuitem" for the actual links (and of course role="menu" for the container).

I wouldn't use the menu role in this contex. Implementing it requires more work, and the benefits for a screen reader user can be minimal or non-existent.

[crhallberg]

@pasitiis Are you talking about the keyboard support expected of a role="menu"? We have some of this code in the confirmation menu PR.

[ckaz]

@pasitiis Are you talking about the keyboard support expected of a role="menu"? We have some of this code in the confirmation menu PR.

Guess we should re-evaluate in the light of @stephanieleary 's comment https://github.com/vufind-org/vufind/pull/2914#issuecomment-1578750127

[ckaz]

To get the css compile running, I had to merge #2922 into this. Also merged dev into it. Pls. review.

[demiankatz]

Thanks, @ckaz -- I'll wait to review this further until we can get #2922 merged so it's easier to focus on the changes specific to this PR.

[demiankatz]

@ckaz, now that #2922 is merged, can you resolve the conflicts here and make sure that none of the unwanted vendor changes carry over into this PR? It's possible you might be able to accomplish this by git revert of whichever commit added the older version of #2922 prior to merging in the dev branch, but I'm not sure if the commit history here is straightforward enough to allow that to work. Please let me know if you need any help straightening it out!

[demiankatz]

Since @ckaz is on holiday at the moment, I decided to take care of resolving conflicts today to keep this moving forward. I believe everything is in good order now, and the full automated test suite is now passing.

@sturkel89, can you take another look at this? See our comments at the very top of this thread for what needs to be tested -- all the subsequent conversation can be ignored as it has to do with fixing problems that were discovered along the way. Ideally, this should be visually identical to the dev branch, so you shouldn't see any differences -- the changes are just intended to improve accessibility for screen readers.

[ckaz]

@demiankatz sorry, I overlooked your comment from last week. I will be on holiday leave from the mid of next week until August.
Thank you for making the changes. Looks good to me.

[demiankatz]

@demiankatz sorry, I overlooked your comment from last week. I will be on holiday leave from the mid of next week until August. Thank you for making the changes. Looks good to me.

Thanks for taking a look, @ckaz -- happy to help!

[sturkel89]

I've rechecked the test branch in all three themes and compared the results to dev.

Everything we identified before has been fixed!

• The visible bullets are now invisible.
• The badges indicating how many items are on a list are circles, not stretched out ovals.
• The badges are aligned with the top of the list name, not center aligned.

My only tiny tweak suggestion is to increase the padding between lines when a list name wraps to multiple lines. It doesn't look too bad in Bootstrap3 or Sandal, but I think the line padding looks kind of tight in Bootprint3. NB: There's no difference between the test branch and the dev branch -- this tight spacing is present in dev, so feel free to disregard this very minor complaint!

Examples:

Bootprint3

Sandal

Bootstrap3

[demiankatz]

Thanks, @sturkel89, for the careful testing. I'm going to merge this now, since the issue you highlight is not changed by this PR and is not directly related to the change proposed here. We should address that as a follow-up action. @crhallberg, is this something you could take a look at when time permits?