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

Update site title when it changes from site settings #18543

Merged
merged 4 commits into from
May 10, 2022

Conversation

Gio2018
Copy link
Contributor

@Gio2018 Gio2018 commented May 9, 2022

Fixes #18484

This PR fixes an issue that prevented the site title in My Site to be updated when it changed from site settings

To test:

  • checkout/build/run this branch
  • Go to My Site -> Site Setitngs
  • Tap on Site Title and update the existing title
  • Navigate back to My Site, and ensure that both the site title and the navigation bar title reflect the new title
  • Still on My Site, tap on the site title, and update the title
  • Make sure that:
    • The spinner appears on the site title while the update is in progress
    • The title is correctly updated when the spinner stops
    • The notice of update title appears at the bottom of the screen (see screenshot below)
updates from Site Settings Updates from My Site

Regression Notes

  1. Potential unintended areas of impact
    None

  2. What I did to test those areas of impact (or what existing automated tests I relied on)
    NA

  3. What automated tests I added (or what prevented me from doing so)
    None
    PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding unit tests for my changes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

…der to update site title when it changes elsewhere
@Gio2018 Gio2018 added this to the 19.9 milestone May 9, 2022
@Gio2018 Gio2018 self-assigned this May 9, 2022
@Gio2018 Gio2018 requested review from twstokes and sla8c May 9, 2022 20:52
Copy link
Contributor

@twstokes twstokes left a comment

Choose a reason for hiding this comment

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

LGTM @Gio2018 - it works as described. Great work! 🎉

I also tested for any regressions related to this issue and found none.

One minor non-blocking observation. There's a brief flash of the title going from an empty string to the new one which is more noticeable when changing the title directly from the modal. I didn't dig to see how complex it would be to smooth that out, but thought I'd bring it up.

Video Screenshot
Simulator.Screen.Recording.-.iPhone.13.-.2022-05-09.at.18.02.17.mp4
Screen Shot 2022-05-09 at 18 03 38

@Gio2018
Copy link
Contributor Author

Gio2018 commented May 10, 2022

Thank you for reviewing @twstokes !

There's a brief flash of the title going from an empty string to the new one which is more noticeable when changing the title directly from the modal

If I understand this correctly, I believe it depends on this code: I think we can leave as it is, because if the connection is not that fast, the spinner might take longer to disappear and it does make sense to blank the underlying title momentarily.

@Gio2018 Gio2018 merged commit cfeca95 into trunk May 10, 2022
@Gio2018 Gio2018 deleted the issue/18484-site-title-updates-mt-site branch May 10, 2022 18:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

My Site: Site title changes through Site Settings not respected on My Site screen
2 participants