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: 'open in new tab' option issue & multiple link control UX issues #50602

Closed
wants to merge 7 commits into from
Closed

fix: 'open in new tab' option issue & multiple link control UX issues #50602

wants to merge 7 commits into from

Commits on May 12, 2023

  1. fix: multiple link control isues

    *** What I've Done 👇👇 ***
    
    ** 1) took a new state 'newValue'/'setNewValue' and took 'value' it's initial default. and replaced the usage of 'value' with this 'newValue' state everywhere in the index.js to make this state the source of truth. we'll need this state for couple of reasons and one being 'updating' the value using 'setNewValue' & another is replace 'onChange' call with 'setNewValue' or use both of them according to our need (more on this in the next steps to make it clear why this new state is taken, see the next steps for more clarity)
    
    ** 2) instead of passing the 'onChange' as prop to 'LinkControlSettingsDrawer', passed 'setNewValue' as prop. Because, the 'onChange' call makes the link popup modal to close without saving the other data properly. In 'settings-drawer.js' the receieved 'setNewValue' passed to the 'Settings' component as 'onChange' prop, this way toggling the 'open in new tab' won't close the modal & also will save the data in 'newValue' state properly.
    
    ** 3) in 'handleSubmit' of 'index.js' removed the if check as looks like it's unnecessary. Because if user decide to submit without making any change to 'url' or 'title' there's no harm in still submitting it, also there's other change can happen like 'open in new tab settings' and user may just wanna submit that. Another thing done it 'handleSubmit' & that is called both 'setNewValue' and 'onChnage' to make sure the 'local state' (newValue) stays sync with the submitted data (otherwise there may be unwanted behaviour where 'link-control component' is used. i.e. 'button block')
    
    ** 4) imported the 'Settings' component inside 'index.js' and used it below the 'LinkPreview' component. used the neccessary css in 'style.scss' to properly style it. In this 'Settings' component's 'onChange' prop, passed a function & made sure the value gets called with both 'setNewValue' and 'onChnge'. This is done for the same reason of keeping the both state sync. This settings component showen there to keep the 'open in new tab option' visible when previewing the link (after initially any richtext is linked by selecting the text & then pasting the link ). This was done to keep it consistent with 'WordPress 6.2 core UI' and looks like many people find it better UX to keep the 'option' there. See these comments for more info: 1) https://github.com/WordPress/gutenberg/issues/49091\#issuecomment-1477057235 2) https://github.com/WordPress/gutenberg/issues/49091\#issuecomment-1477084979 (see the video in this comment for more clarity) 3) https://github.com/WordPress/gutenberg/issues/49091\#issuecomment-1477097292
    
    ** 5) finally 2 extra works done related to keeping the settings panel visible when opening the modal as seems like it's a better UX to keep this option visible rather than keeping them hidden and then requiring 1 extra click to make them open. To achieve this set the initial default value to 'true' for 'settingsOpen' useState & removed the 'setSettingsOpen(false)' from 'stopEditing' function.
    
    *** Thanks 😊😊 ***
    hz-tyfoon committed May 12, 2023
    Configuration menu
    Copy the full SHA
    9a01925 View commit details
    Browse the repository at this point in the history

Commits on May 15, 2023

  1. fix: clicking 'cancel' button still shows the updated change

     - fixed: when making some change after opening the 'link control modal' & without applying/submitting it, when the 'cancel' button is clicked it show's that the changes already applied even though it's cancelled (although the updated changes aren't really applied but, still it's a misleading UI so fixed it)
    hz-tyfoon committed May 15, 2023
    Configuration menu
    Copy the full SHA
    2b35bad View commit details
    Browse the repository at this point in the history

Commits on May 16, 2023

  1. reverted 'fix/item: 4' (see the description for details) as looks lik…

    …e it's better to handle it separately from 'PR 50602'
    
    *** What was the 'fix/item: 4' that's reverted in this commit? here's what I wrote about it in my description 👇👇 ***
    
    "imported the 'Settings' component inside 'index.js' and used it below the 'LinkPreview' component. used the neccessary css in 'style.scss' to properly style it. In this 'Settings' component's 'onChange' prop, passed a function & made sure the value gets called with both 'setNewValue' and 'onChnge'. This is done for the same reason of keeping the both state sync. This settings component showen there to keep the 'open in new tab option' visible when previewing the link (after initially any richtext is linked by selecting the text & then pasting the link ). This was done to keep it consistent with 'WordPress 6.2 core UI' and looks like many people find it better UX to keep the 'option' there. See these comments for more info: this comment, also this comment with a video demonstration & this comment"
    hz-tyfoon committed May 16, 2023
    Configuration menu
    Copy the full SHA
    c5eb7dc View commit details
    Browse the repository at this point in the history
  2. reverted 'fix/item: 5' (see the description for details) as looks lik…

    …e it's better to handle it separately from 'PR 50602'
    
    *** What was the 'fix/item: 5' that's removed in this commit? here's what I wrote about it in my description 👇👇 ***
    
    "finally 2 extra works done related to keeping the settings panel visible when opening the modal as seems like it's a better UX to keep this option visible rather than keeping them hidden and then requiring 1 extra click to make them open. To achieve this set the initial default value to 'true' for 'settingsOpen' useState & removed the 'setSettingsOpen(false)' from 'stopEditing' function."
    hz-tyfoon committed May 16, 2023
    Configuration menu
    Copy the full SHA
    e3ce2c6 View commit details
    Browse the repository at this point in the history
  3. revert / (slight refactor): kept the prop name 'onChange' rather than…

    … 'setNewValue' but passed the 'setNewValue function' as suggested by dave
    
    I inititally renamed both the prop 'name and value' to 'setNewValue' so that it prevents any confusion being created (like, is the recieved 'onChange' prop in 'settings-drawe.js' is the same one which was recieved in the 'index.js'?) But, looks like that's not something to worry about at this moment. Looks like it's better to have that disscussion separately from this PR
    hz-tyfoon committed May 16, 2023
    Configuration menu
    Copy the full SHA
    ba870b2 View commit details
    Browse the repository at this point in the history
  4. revert / (slight refactor): reverted back to old behaviour (selecting…

    … suggested links closes the modal) as per review
    
    Why?
    
    To keep concerns separate. Think It'd be better if this is disscussed separately & this PR shouldn't look to change this behaviour. more by dave here: https://github.com/WordPress/gutenberg/pull/50602\#discussion_r1194818569
    hz-tyfoon committed May 16, 2023
    Configuration menu
    Copy the full SHA
    e33dffa View commit details
    Browse the repository at this point in the history
  5. revert: used 'value' rather than 'newValue' as per review

    why?
    
    We can disscuss which one is better in this case separately from this PR. see this https://github.com/WordPress/gutenberg/pull/50602\#discussion_r1194821669
    hz-tyfoon committed May 16, 2023
    Configuration menu
    Copy the full SHA
    05924eb View commit details
    Browse the repository at this point in the history