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

DDFLSBP-637 - Change advanced search input element into and img element #1359

Merged
merged 6 commits into from
Aug 12, 2024

Conversation

Dresse
Copy link
Contributor

@Dresse Dresse commented Aug 1, 2024

Link to issue

https://reload.atlassian.net/browse/DDFLSBP-637

Description

Changed the advanced-search input element into a button element containing an image element.

Screenshot of the result

Screenshot 2024-08-02 at 11 37 24

Additional comments or questions

This PR has a sister PR here: danskernesdigitalebibliotek/dpl-design-system#701

Dresse added 3 commits August 1, 2024 13:45
Input elements with type "image" should only be used for making
graphical submit buttons.
as img elements are non interactive.
I have instead opted for keeping the input element and assigned the
role of "combobox" and giving it a aria-controls property, which are
set to the element that is showing the dropdown.
approach, as it showed the input field as an actual input field.
Instead I have opted to use a Button, containing an image element.
/>
aria-controls="search-header-dropdown"
>
<img src={expandIcon} alt={t("searchHeaderDropdownText")} />
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe the alt text should be empty. There is already an explanatory aria-label on the button itself, and the icon is purely decorative.

Please also remember to update the design system:
https://github.com/danskernesdigitalebibliotek/dpl-design-system/blob/61b0e63f93d93328401a3e6b8e0f18089a7d4be7/src/stories/Blocks/header/Header.tsx#L112

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense. I have set the alt tab to empty now.

I will look into updating the design system 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kasperbirch1

I have updated dpl-design-system: danskernesdigitalebibliotek/dpl-design-system#701

I just added the aria-label. Do you think that is enough, or are more changes needed here?

Copy link
Contributor

@kasperbirch1 kasperbirch1 left a comment

Choose a reason for hiding this comment

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

I have a comment you can consider ☺️

purposes, and therefore the aria-label should be enough.
@Dresse Dresse merged commit 367ac59 into develop Aug 12, 2024
20 checks passed
@Dresse Dresse deleted the DDFLSBP-637-change-advance-search-dropdown-to-img branch August 12, 2024 07:42
@Dresse Dresse restored the DDFLSBP-637-change-advance-search-dropdown-to-img branch August 12, 2024 07:45
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.

5 participants