Fix toggle new tab does not persist changes to text input in Link Control

[hz-tyfoon]

Updated version of the PR is this 👉 #50602. Probaly gonna close this PR soon, but keeping it open for now to get some feedback as to if this one is a a better candidate just for a cherry pick to wp/6.2 minor release.

The mentioned updated PR is created as this one doesn't maintain the current state/UI/UX of the link-control component. See the conversations & the videos shown there for more clarity

This PR fixes issues: #45741 & #43144

Here's what I did in my code to fix this issue👇:

** step 1: took a new state 'newValue' and used the 'recieved 'value' prop' as default.
** step 2: replaced the 'value' with 'newValue' state everywhere. It's help us to change the 'value' without causing any mutation using 'setNewValue' when we need it
** step 3: passed 'setNewValue' function to 'LinkControlSettingsDrawer' component to be able to change the value from inside that component
** step 4: in 'settings-drawer.js' file used the passed 'setNewValue' function to update 'just the settings' and keeping the other values to make sure no mutation happen to the original 'value' prop
** step 5: removed 'onChange' entirely from 'LinkControlSettingsDrawer' as 'this onchange call is the culprit' that causes the link modal to close. also removed passing the 'onChange' as a prop to 'LinkControlSettingsDrawer' component in the 'index.js' file as it's no longer used

[hz-tyfoon]

Update: fixed the issue visible in the Video timestamp: 1:16 in here: de648d1

Here's a video demonstration of the fix.
[Note: the video also has sound, but can be understandable just by watching]

link.control.fix.demonstration.mp4

[paaljoachim]

Thank you @hz-tyfoon

@getdave Dave what are your thoughts here?

[hz-tyfoon]

Silly me, found an issue watching my own video 😅😅 where the Amazon link didn't update to "open in new tab" after submission (Video timestamp: 1:16). Identified the issue in the codebase and also fixed it in: de648d1

[hz-tyfoon]

the issue shown in the video is fixed in last 2 commits (9409630491 & 328d933c97).

issue_with_button_block.mp4

[getdave]

Thank you for the PR. It is on my list to review.

[getdave]

@hz-tyfoon I think the best bet is you rebasing the fix with upstream trunk. I can't do this as I don't have access to your repo. We should target the fix to trunk and then backport it to 6.2 branch.

[hz-tyfoon]

Thanks @getdave for replying. 😊

Just noting few things:

I believe My repo is public and U can access it. But no worries, I'll try to resolve the conflicts later or create a new PR later on the trunk if resolving/rebasing seems harder.

But, when I read this comment & the video there it looked to me that current state of link-control (the way it's now at trunk branch) looks a bit confusing to many people compared to how it is now in the core WP 6.2.
And that was another reason why I created the PR @ wp/6.2 branch.

Looks to me if I proceed from the trunk the the UI/UX of link-control would be different than when the issues (i.e.: #45741 & #43144) were reported.

1 thing poped into my head & that is to try to get some eyes on this and get peoples feedback on the UX comparison between wp 6.2 core version of the link-control & trunk version of the link-control.
What do U think? 🤔

Any guidance would be appreciated as to how I should proceed in the light of this UI/UX change.
Thanks again.

[getdave]

Ah yes I'd forgotten that thanks for reminding me.

So for the 6.3 cycle we're anticipating some changes to the <LinkControl> component which were not ready for 6.2. Therefore yes there is a disparity in the UI between trunk and 6.2.

So I think we'll need to have x2 fixes. One to be backported to 6.2 and then another to merge into trunk.

[hz-tyfoon]

Thanks for replying so quick 🚀.
I'll create another PR for trunk soon (hopefully within next 2/3 days).
Do U think this PR can go for wp/6.2 backport minor release?

Meanwhile, It'd be great if we get some more feedback, review or comment from other people too.
Thanks 😊 🙏 again.

[getdave]

Meanwhile, It'd be great if we get some more feedback, review or comment from other people too.

I think the diff is too messed up for that right now. I was planning to solve this in 6.3 by requiring that all changes to the link value have to be submitted with the Apply button.

But a fix for 6.2 would need to be more along the lines of what you propose here.

[hz-tyfoon]

Ok then.
Do U think switching the base branch back to wp/6.2 a good idea?
Because, planning for another PR for trunk (mentioned in the earlier comment). 😊
What U think?

[draganescu]

Hi @hz-tyfoon let's try to fix this PR because no one can see the code changes to review.
At this point I am not sure what happened but the best path forward is:

• copy the files with your changes in a new place
• in your fork make sure trunk is up to date
• switch to issue-45741_for_wp/6.2 branch and git reset --hard origin/trunk
• reapply your changes from where you saved them
• push to your fork

Then this PR will show the diff so we can review the code and move on. Let me know how it went :)

[hz-tyfoon]

Thanks @draganescu 😊
I did what U suggested & the fix still works but looks like it brought the link control back to it's old UI (the UI that's with wp core 6.2).
should I create a new PR where the fix doesn't change the current UI and just fix those 2 issues?
Or, do that in this PR and force push the commit again?

here's the old 4 commits(if needed) before the force push: 1st. f545717 2nd. de648d16 3rd. 9409630 4th. 328d933c978b3dffd07c82157d6f589d0157d263

[note: we had a brief chat about it on today's meeting]

[getdave]

If we go with this route we can curry the function to pre-apply the handlers. That way you can just pass the new value to the curried function.

[hz-tyfoon]

refactored the __newOnChangeForLinkControl to call it just with the values in 91306d3.

[getdave]

Thank you for working on this.

What I'm seeing is that we're going to try and store the unsubmitted changes in internal component state.

The key is that value (the controlled prop) should be the source of truth and that any internal state should only exist until the point of submission. If it's not submitted then the internal state should be reset to whatever value is passed in.

I'm going to dive into this in more detail here before I make further suggestions.

[hz-tyfoon]

Thanks @getdave 😊 for the review. I think U got the right idea about what I tried to do here.
[noting: little occupied at this moment, I'll check the function currying idea and try to apply it soon as I get the chance]

[hz-tyfoon]

Updated the PR to call newOnChangeForLinkControl with just the values.

again noting that: at this point this PR's work changing the UI of link-control and making it look like the older version (the one currently with core wp 6.2, the apply & cancel buttons are gone).
So, shouldn't it be that all the UI changes stays how they are in current trunk and just the 2 issues get fixed maintaining the current UI? If yes, Then I'll try to update the PR accordingly. 😊

[hz-tyfoon]

Updated version of the PR is this 👉 #50602. Probaly gonna close this PR soon, but keeping it open for now to get some feedback as to if this one is a a better candidate just for a cherry pick to wp/6.2 minor release.

The mentioned updated PR is created as this one doesn't maintain the current state/UI/UX of the link-control component. See the conversations above & the videos shown there for more clarity

[draganescu]

@hz-tyfoon I don't understand what is the difference between the two PRs :)

[hz-tyfoon]

@draganescu if you apply this PR's patch & then try it U'll find the UI has been changed & that's because my initial PR's base was taken from wp/6.2 branch & not trunk.
And after doing hard reset I & then applying the changes as U suggested all the latest works of the link-control was gone. So, I created this updated PR 👉 #50602

Now,
if this PR gets merged (may be unlikely) before the updated one, Then we can cherry pick this PR's work into wp/6.2 branch & that way the UI of the link-control in WordPress 6.2 minor release will remain the same with the mentioned issues also getting fixed.
But, if this one gets closed and if we want to 'cherry pick' the fixs into wp 6.2 minor release from the updated PR (when/if that is merged) then that minor release probably will have a totally different UI than the wp 6.2.0 version, which may be something we don't want.

But, if we decide that we don't want to cherry pick this fixs into 6.2 minor release then we can close this PR & everything's ok.

I tried to explain my thinking & hope this wasn't confusing. 😊

[getdave]

I believe upcoming changes to the Nav block are going to necessitate changes to the Link UI. As a result let's hold off on merging this PR until we're sure on the desired UX for the control.

I think the idea is along the right lines but let's take our time to get this right. I'll circle back here asap.

Thank you again for your patience.

[richtabor]

I believe upcoming changes to the Nav block are going to necessitate #49091.

Correct. Let's move to a CheckboxControl and require "Save" click to make changes—details in #50890.

[getdave]

I believe this is now resolved by #50945.

@hz-tyfoon Apologies that this got superseded. If you're interested there are lots of open tasks on the tracking Issue for the refresh project.