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

fix: #1121 breadcrumb component not routing #1169

Merged
merged 3 commits into from
Feb 8, 2024

Conversation

J0taFerreira
Copy link
Contributor

@J0taFerreira J0taFerreira commented Feb 1, 2024

  • Fixed breadcrumb component not routing.

The breadcrumb component now receives the home as a prop, in the primevue breadcrumb component documentation has exemples using the name "route" for the the path but the component it self expects "item.url". Tried using "route" and "path" as the routeItems object key but it did not work, "url" worked though.

primefaces/primevue@ad9b437#diff-33bf4bb142b615a34557b44002c2eb3ac4393bd3fa4099aede38fabbf4e5527f
https://primevue.org/breadcrumb/

@MCatherine1994
Copy link
Contributor

MCatherine1994 commented Feb 1, 2024

Hi Jota, I think there is an example in the primevue documentation https://primevue.org/breadcrumb/#router, we can follow that. So change the route item "path" to "route". Also need to change all the places that is using ".path"
image

And update the Breadcrumb component in the frontend/src/components/common/PageTitle.vue file to something like below, it should work. Then we don't have to add additional home props. Just a reminder, in this example we might don't need all of it, like some classname, let's just add what we need. Thanks so much!!
image

@J0taFerreira
Copy link
Contributor Author

J0taFerreira commented Feb 2, 2024

Hi Jota, I think there is an example in the primevue documentation https://primevue.org/breadcrumb/#router, we can follow that. So change the route item "path" to "route". Also need to change all the places that is using ".path" image

And update the Breadcrumb component in the frontend/src/components/common/PageTitle.vue file to something like below, it should work. Then we don't have to add additional home props. Just a reminder, in this example we might don't need all of it, like some classname, let's just add what we need. Thanks so much!! image

"route" or "path" will work using the template , but it will break styling. I need to check to be sure

@J0taFerreira
Copy link
Contributor Author

Hi Jota, I think there is an example in the primevue documentation https://primevue.org/breadcrumb/#router, we can follow that. So change the route item "path" to "route". Also need to change all the places that is using ".path" image
And update the Breadcrumb component in the frontend/src/components/common/PageTitle.vue file to something like below, it should work. Then we don't have to add additional home props. Just a reminder, in this example we might don't need all of it, like some classname, let's just add what we need. Thanks so much!! image

"route" or "path" will work using the template , but it will break styling. I need to check to be sure

using the template sintax works with "path" as the routeItems key but will break the styling. Should I make the change and focus on the styling?

@J0taFerreira J0taFerreira force-pushed the fix/1121-breadcrumb-component-not-routing branch from 5d8086b to 6e48a5b Compare February 2, 2024 19:44
NickSaglioni
NickSaglioni previously approved these changes Feb 2, 2024
Copy link
Contributor

@NickSaglioni NickSaglioni left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@MCatherine1994
Copy link
Contributor

Just add a note here, seems like Primevue breadcrumb can get the item.url and make the link works, so we changed "path" to be "url": primefaces/primevue@ad9b437#diff-33bf4bb142b615a34557b44002c2eb3ac4393bd3fa4099aede38fabbf4e5527f

@MCatherine1994
Copy link
Contributor

MCatherine1994 commented Feb 2, 2024

Hi Jota, I think there are two more places is using ".path", we have to change them, otherwise like the sidebar navigation is not working
Screen Shot 2024-02-02 at 2 31 11 PM

Hello Catherine! I might be wrong but these two that you mentioned are using the vue router path property and not our objects. The sideNav is working fine when you click the add user permission, add application admin fails because it does not have a route yet, the route for that is in Nick's PR.
sidenavRoutePrint

frontend/src/router/index.ts Outdated Show resolved Hide resolved
@MCatherine1994
Copy link
Contributor

MCatherine1994 commented Feb 8, 2024

Hi Jota, please check the comment I put in the pr in the common repo, bcgov/nr-theme#155 (review). We don't need to add these extra custom class, let's remove them, and upgrade to the latest theme package v1.8.4. Thanks!!

ianliuwk1019
ianliuwk1019 previously approved these changes Feb 8, 2024
Copy link
Collaborator

@ianliuwk1019 ianliuwk1019 left a comment

Choose a reason for hiding this comment

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

Approve. Just wondering if the addition of "$light-focus" in .scss is intended or is a left over?

@J0taFerreira
Copy link
Contributor Author

Approve. Just wondering if the addition of "$light-focus" in .scss is intended or is a left over?

thanks Ian, just removed

Copy link

sonarqubecloud bot commented Feb 8, 2024

Quality Gate Passed Quality Gate passed for 'nr-forests-access-management_admin'

Issues
0 New issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@J0taFerreira J0taFerreira merged commit f14c878 into main Feb 8, 2024
10 checks passed
@J0taFerreira J0taFerreira deleted the fix/1121-breadcrumb-component-not-routing branch February 8, 2024 17:24
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.

4 participants