Skip to content

Replace in-place links with buttons#2196

Merged
tdonohue merged 7 commits intoDSpace:mainfrom
atmire:w2p-101198_in-place-links_contribute-main
May 10, 2023
Merged

Replace in-place links with buttons#2196
tdonohue merged 7 commits intoDSpace:mainfrom
atmire:w2p-101198_in-place-links_contribute-main

Conversation

@LotteHofstede
Copy link
Copy Markdown
Contributor

@LotteHofstede LotteHofstede commented Apr 19, 2023

References

Description

This PR replaces instances of empty/in-place links with buttons

Instructions for Reviewers

Compare the following links/buttons before and after this PR:

  • The "View more" button on the /community-list page
  • The "View more" and "Collapse" links on the item page for the download list
  • The "View more" and "View less" links on the publication page for the author (metadata-representation) list
  • The "View more" and "View less" links on the item page for the related items list
  • The search bar magnifying glass icon in the header

Checklist

  • My PR is small in size (e.g. less than 1,000 lines of code, not including comments & specs/tests), or I have provided reasons as to why that's not possible.
  • My PR passes ESLint validation using yarn lint
  • My PR doesn't introduce circular dependencies (verified via yarn check-circ-deps)
  • My PR includes TypeDoc comments for all new (or modified) public methods and classes. It also includes TypeDoc for large or complex private methods.
  • My PR passes all specs/tests and includes new/updated specs or tests based on the Code Testing Guide.
  • If my PR includes new libraries/dependencies (in package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.
  • If my PR includes new features or configurations, I've provided basic technical documentation in the PR itself.
  • If my PR fixes an issue ticket, I've linked them together.

@artlowel artlowel added bug accessibility 1 APPROVAL pull request only requires a single approval to merge labels Apr 19, 2023
@artlowel artlowel added this to the 7.6 milestone Apr 19, 2023
@tdonohue tdonohue self-requested a review April 20, 2023 14:24
Copy link
Copy Markdown
Member

@tdonohue tdonohue left a comment

Choose a reason for hiding this comment

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

@LotteHofstede : Overall the changes look good. But, I'm noticing that the searchbox in the header is now misaligned...and the search icon in the header seems to have moved further away from the globe (just more whitespace between them).

search-nav-alignment

I'm testing in production mode (yarn build:prod; yarn serve:ssr).

I thought it might be a cached stylesheet at first, but tried in several browsers (Chrome and Firefox) and see the same in both. I also did a complete rebuild (including a yarn clean) and still see the same behavior.

@LotteHofstede
Copy link
Copy Markdown
Contributor Author

@tdonohue Thanks for the review!
I was able to reproduce the issue, so it definitely wasn't your cache!

The issue was caused by the fact that I renamed a custom CSS class but forgot to change it in the HTML for that particular instance. It should be fixed now!

image

@tdonohue tdonohue self-requested a review May 4, 2023 14:55
Copy link
Copy Markdown
Member

@tdonohue tdonohue left a comment

Choose a reason for hiding this comment

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

@LotteHofstede : Looks good now overall! And the CSS issues are fixed on my end.

However, I found one last (very minor) accessibility issue. We have a new <button> that has no text to describe it. It should be quick to fix (see comment below).

Comment thread src/app/search-navbar/search-navbar.component.html Outdated
@LotteHofstede LotteHofstede requested a review from tdonohue May 5, 2023 08:04
formControlName="query" type="text" placeholder="{{searchExpanded ? ('nav.search' | translate) : ''}}"
class="d-inline-block bg-transparent position-absolute form-control dropdown-menu-right p-1" [attr.data-test]="'header-search-box' | dsBrowserOnly">
<a class="submit-icon" [routerLink]="" (click)="searchExpanded ? onSubmit(searchForm.value) : expand()" [attr.data-test]="'header-search-icon' | dsBrowserOnly">
<button class="submit-icon btn btn-link btn-link-inline" aria-label="Submit search" type="button" (click)="searchExpanded ? onSubmit(searchForm.value) : expand()" [attr.data-test]="'header-search-icon' | dsBrowserOnly">
Copy link
Copy Markdown
Member

@tdonohue tdonohue May 5, 2023

Choose a reason for hiding this comment

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

@LotteHofstede : Sorry, I should have been more clear. This aria-label should use an i18n key (e.g. the existing nav.search or a new key if you'd rather), so that it can be easily translated.

So, I think this needs to be something like:
[attr.aria-label]="'nav.search' | translate"

Otherwise, our accessibility label can only ever be in English.

This is the same way that all other navbar buttons (login, etc) have translatable ARIA labels (see auth-nav-menu.component.html for example).

@tdonohue tdonohue self-requested a review May 10, 2023 17:27
Copy link
Copy Markdown
Member

@tdonohue tdonohue left a comment

Choose a reason for hiding this comment

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

👍 Thanks @LotteHofstede ! This looks good to me now & verified it works and passes accessibility checks.

@tdonohue tdonohue merged commit c914b42 into DSpace:main May 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

1 APPROVAL pull request only requires a single approval to merge accessibility bug

Projects

No open projects
Status: ✅ Done

Development

Successfully merging this pull request may close these issues.

Multiple components use "in place links" instead of buttons

3 participants