WebPartTitle Control Fails to Update After Initial Save

[ferrarirosso]

Category

[ ] Enhancement

[x] Bug

[ ] Question

Version

v3.19.0

Affects also previous versions.

Expected Behavior

The WebPart title should be editable every time the WebPart is in edit mode, not just during the initial setup.

Observed Behavior

• The WebPart title can only be set once, during the initial addition of the WebPart.
• The event appears to propagate correctly, but the WebPart does not re-render, leading to the observed faulty behavior.
• Note: Users have reported that this faulty behavior started appearing a couple of days ago.
• Forcing a re-render by adding this.render() resolves the issue, but this would require updating all existing components. Additionally, this requirement is not documented as mandatory.

updateProperty: (value: string) => { this.properties.title = value; this.render(); }}

Steps to Reproduce:

1. Scaffold a new SPFx React solution using SPFx v1.19.0.
2. Ensure you are using Node.js v18.19.1.
3. Implement the WebPartTitle Control as per the PnP WebPartTitle documentation.
4. Add the WebPart to a SharePoint page.
5. Publish the page.
6. Edit the page and attempt to update the WebPart title.

[ferrarirosso]

I have investigated the problem further

The issue arises from this commit #1672

The problem lies in the fact that changing the textarea element to use value instead of defaultValue makes it a controlled component in React.

Please refer to the React documentation on React Js Uncontrolled Components - Default values

Since this change breaks production code and would require modifications to all solutions using the WebPartTitle component, I would suggest reverting this change. I am not sure of the original intent behind changing this in relation to Issue #1672.

Please advise if I should proceed with this change

[ferrarirosso]

Any news here ?
@AJIXuMuK

[stefan-at-ilionx]

We're experiencing the same issue. We can't update beyond 3.16.2 because of this.

[joelfmrodrigues]

@t0mgerman, is there any chance you could help us get a better understanding of why the change was done a while back as you seem to have created the PR? I know it was a while back and it's hard to remember/review, which is why I am reaching out to you as I went through the PR details but was not sure why this was done. It seemed to me that you were trying to standardise the use of the value and defaultValue properties across different controls, but I could be wrong.

[t0mgerman]

@joelfmrodrigues We have experienced some issues after these changes were merged too, so I can only hold my hands up there and apologise! 😔

I think that was sort of my intention, but I don't actually recall any need to modify the WebPartTitle component - I think that may have been a change made in error.

The goal of the offending commit was to firstly start an implementation of client-side formula validation in the DynamicForm component - but in doing so, to also try and make better sense of the use of properties like value and defaultValue in the DynamicForm and DynamicField components, as there were at times not just those properties but changedValue etc. Sometimes a defaultValue property would be used against a value prop for a control and things like that. So it seemed a bit messy / inconsistent. I suspect I've changed things in the WebPartTitle component by mistake while making those changes and I've not spotted the above issue as I principally tested the DynamicForm component before submitting a PR.

I've found other problems with the implementation that I wanted to redress, but I need to get my dev branch up to date locally and understand what fixes have been made since.

[joelfmrodrigues]

@t0mgerman absolutely no need to apologise! Any contributions are welcome and we are all doing this in our own free time so no finger-pointing when there are issues 😊 I will merge the PR for this issue then.
If you need any help with getting your local branch up to date feel free to reach out to us. There is a relatively new sync option on GitHub that can sync your fork with the main repo and then you can just sync locally with your fork version, which makes things fairly simpler for most scenarios.

[joelfmrodrigues]

Hey @ferrarirosso thanks for reporting this, the PR to fix this issue is now merged. If you have the chance to test the beta release, please let us know if the issue has been fixed.

[ferrarirosso]

will check it later and give you my feedback

[ferrarirosso]

• Tested current WP which doesn't work
• npm i @pnp/[email protected]
• Tested again, it works !

It's Christmas soon 🎅

Thanks @joelfmrodrigues

[joelfmrodrigues]

@ferrarirosso many thanks for confirming