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

Upgrade react-application-breadcrumb to SPFx v1.20.0 #1419

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

tom-daly
Copy link
Contributor

@tom-daly tom-daly commented Oct 3, 2024

By submitting this pull request, you agree to the contribution guidelines

If you aren't familiar with how to contribute to open-source repositories using GitHub, or if you find the instructions on this page confusing, sign up for one of our Sharing is Caring events. It's completely free, and we'll guide you through the process.

Q A
Bug fix? no
New feature? no
New sample? no
Related issues? #1412

What's in this Pull Request?

  • Rebuild as SPFx v1.20.0
  • Remove Office UI Fabric replace with Fluent UI
  • fix critical typescript issues
  • validate functionality

@tom-daly tom-daly changed the title Upgrade react application breadcrumb Upgrade react application breadcrumb to SPFx v1.20.0 Oct 3, 2024
@tom-daly tom-daly changed the title Upgrade react application breadcrumb to SPFx v1.20.0 Upgrade react-application-breadcrumb to SPFx v1.20.0 Oct 3, 2024
@Adam-it Adam-it self-assigned this Oct 5, 2024
@Adam-it
Copy link
Contributor

Adam-it commented Oct 5, 2024

Thank you for opening a PR to our samples repo and helping us upgrade our project 👍
We will try to review this PR ASAP

Copy link
Contributor

@Adam-it Adam-it left a comment

Choose a reason for hiding this comment

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

@tom-daly Awesome work so far 👏👏👏
I really like the changes and the refactor done along the way. You Rock 🤩
Unfortunately I noticed a small left over we need to sort out before we merge 👍. Please do give it a double check 🙏

@@ -2,7 +2,7 @@ declare interface ISiteBreadcrumbStrings {
Title: string;
}

declare module 'siteBreadcrumbStrings' {
declare module 'SiteBreadcrumbApplicationCustomizerStrings' {
Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately, after this change we should also update config/config.json and change the localizedResources to the new name as well

currently without this change if I run the gulp bundle --ship it will crash

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure how that happened because I installed it and tested it. I've just put the name back to the original, checked the build and deployed it out to my test site to ensure it was still working. I cleared everything out and re-added it to ensure it was fresh.

image

@Adam-it Adam-it marked this pull request as draft October 11, 2024 18:54
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.

2 participants