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

[web] Fix broken storage links #877

Merged
merged 5 commits into from
Nov 17, 2023
Merged

[web] Fix broken storage links #877

merged 5 commits into from
Nov 17, 2023

Conversation

dgdavid
Copy link
Contributor

@dgdavid dgdavid commented Nov 17, 2023

Problem

During migration to PF5, specifically when moving to the new PF/Dropdown, a few options were unintentionally broken when their href prop were not properly renamed to to.

This makes the PF/MenuItem to use a <button> instead of <a> inside an <li> when building the menu item, assigning the href HTML attribute to the latest, which has no effect at all.

As a result, users cannot navigate to these options through the UI, as it was the case in https://bugzilla.suse.com/show_bug.cgi?id=1217281

Solution

Fix the issue by using the right prop and improve a bit the unit test.

Testing

  • Updated unit test
  • Tested manually too

We've detected few wrong storage menu items where the `href` attribute is
assigned to the `li` HTML component instead to a MISSING `a` element.
The root of the problem it's in the props passed to PF/MenuItem through
Agama/Storage/ProposalPageOptions --> Agama/PageOptions.Option ->
PF/DropdownItem. We are sending an `href` instead of `to` which makes
PF/MenuItem deliver a <li href='..'> holding a <button> instead of a <li>
holding a <a href='...'>

See https://github.com/patternfly/patternfly-react/blob/7d2fd2678f993459f67a04d173a8d16c16ef50a3/packages/react-core/src/components/Menu/MenuItem.tsx#L139

This was broken during the migration to PF5, specifically when moving to
the new PF/Dropdown where these `href` props were overlooked and not
renamed to `to`.

See 2edc4e2#diff-88232d7f99420122a9e8c24ae183bcf9a9b98451cbb5db44dea461a2d5ed6727

So, this change makes the test more robust by checking tha the
"menuitem" element has the expected "href". Subsequent commit will fix
the problem.
By using the right prop for setting the link href attribute.
@coveralls
Copy link

coveralls commented Nov 17, 2023

Coverage Status

coverage: 75.538%. remained the same
when pulling 3b418b0 on fix-storage-menuitems
into 7785807 on master.

@dgdavid dgdavid merged commit c9784cf into master Nov 17, 2023
2 checks passed
@dgdavid dgdavid deleted the fix-storage-menuitems branch November 17, 2023 14:41
@imobachgs imobachgs mentioned this pull request Dec 2, 2023
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.

3 participants