Skip to content

Sync: Update notification to include Hostname #1412

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.

Sign up for GitHub

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

Jump to bottom
Merged
epeicher merged 3 commits into from
May 20, 2025
Merged

Sync: Update notification to include Hostname #1412

epeicher merged 3 commits into from
May 20, 2025

Conversation

@epeicher
Copy link
Contributor

@epeicher epeicher commented May 19, 2025 •
edited
Loading

Related issues

Proposed Changes

  • Add the Hostname of the connected site to the Push Notification
  • Add the Hostname of the connected site to the Pull Notification
Operation Before After
Pull CleanShot 2025-05-19 at 18 19 18@2x CleanShot 2025-05-19 at 17 13 33@2x
Push CleanShot 2025-05-19 at 18 22 40@2x CleanShot 2025-05-19 at 17 00 05@2x

Testing Instructions

  • Apply this patch and run npm start
  • Navigate to the Sync tab and connect a site or use one of your connected sites
  • Click on Push
  • Check the notification includes the URL of the connected site
  • Click on Pull
  • Check the notification includes the URL of the connected site

Pre-merge Checklist

  • Have you checked for TypeScript, React or other console errors?

@epeicher epeicher changed the title Push: Update notification to include Hostname Pull/Push: Update notification to include Hostname May 19, 2025
@epeicher epeicher changed the title Pull/Push: Update notification to include Hostname Sync: Update notification to include Hostname May 19, 2025
@epeicher epeicher requested a review from a team May 19, 2025 16:13
gcsecsey
gcsecsey approved these changes May 19, 2025
View reviewed changes
Copy link
Contributor

@gcsecsey gcsecsey left a comment

Choose a reason for hiding this comment

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

Thanks for making these changes @epeicher, the notifications are much more descriptive now! 🙌

These changes LGTM and work as described. 🚢

katinthehatsite
katinthehatsite reviewed May 20, 2025
View reviewed changes
src/hooks/sync-sites/use-sync-push.ts
body: syncPushState.isStaging
? __( 'Staging has been updated' )
: __( 'Production has been updated' ),
body: sprintf(
Copy link
Contributor

@katinthehatsite katinthehatsite May 20, 2025

Choose a reason for hiding this comment

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

I am wondering if we should add a comment for translators here what % refers to so that they know it is a site URL. Mostly thinking that in some languages a "site" can be masculine or feminine so it might be useful for translators to know that it is a site and so that they can apply the right translation.

Copy link
Contributor Author

@epeicher epeicher May 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Good point, I'm adding a comment for translators in 8a5041d. Thanks for the suggestion!

katinthehatsite
katinthehatsite approved these changes May 20, 2025
View reviewed changes
Copy link
Contributor

@katinthehatsite katinthehatsite left a comment

Choose a reason for hiding this comment

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

The changes look good and notifications are much clearer now. Thanks for making these improvements. I added one minor comment 👍

Screenshot 2025-05-20 at 9 57 18 AM

ivan-ottinger
ivan-ottinger approved these changes May 20, 2025
View reviewed changes
Copy link
Contributor

@ivan-ottinger ivan-ottinger left a comment

Choose a reason for hiding this comment

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

Nice! The changes look good and provide a great improvement to the notifications!

One thing we could consider is to shorten the URL with … if it is too long, e.g. in a case like this:

Markup on 2025-05-20 at 11:05:31

But that would probably work for the Pull operation only - since the Push notification has the address displayed in the beginning. 🤔

@epeicher
Copy link
Contributor Author

epeicher commented May 20, 2025 •
edited
Loading

One thing we could consider is to shorten the URL with … if it is too long. But that would probably work for the Pull operation only - since the Push notification has the address displayed in the beginning. 🤔

Mmmm... I personally don't like the … pattern, but I understand it is required for very long strings. I would say we can create a follow-up task for that if we receive further feedback about it being too wordy. What do you think @ivan-ottinger?

Update: I will merge this for now as it improves the notification a bit, we can always create follow-ups for further improvements 😄

@epeicher epeicher merged commit 6cdc351 into trunk May 20, 2025
12 checks passed
@epeicher epeicher deleted the stu-510-studio-sync-update-notification-to-identify-the-site branch May 20, 2025 09:35
@ivan-ottinger
Copy link
Contributor

ivan-ottinger commented May 20, 2025

I personally don't like the … pattern, but I understand it is required for very long strings.

Yeah, similarly. I would like to see the full address in general. It just when it is too long it made the notification not that pretty. Regardless, at least full information is there.

I would say we can create a follow-up task for that if we receive further feedback about it being too wordy. What do you think @ivan-ottinger?

That sounds good to me. 👍🏼

sejas
sejas reviewed May 20, 2025
View reviewed changes
Copy link
Member

@sejas sejas left a comment

Choose a reason for hiding this comment

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

The notification look great. Thanks for updating them.
I think it's ok to display the whole domain.

Screenshot 2025-05-20 at 11 39 38

src/lib/url-utils.ts
const parsedUrl = new URL( url );
return parsedUrl.hostname;
} catch ( error ) {
return '';
Copy link
Member

@sejas sejas May 20, 2025

Choose a reason for hiding this comment

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

@epeicher , should we return the whole url param if we cannot parse it?

Copy link
Contributor Author

@epeicher epeicher May 20, 2025

Choose a reason for hiding this comment

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

Thinking again about it, I think it is a better UX than displaying an empty string. Let me create a quick PR for that.

Copy link
Contributor Author

@epeicher epeicher May 20, 2025

Choose a reason for hiding this comment

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

I have created #1420 for that

@epeicher epeicher mentioned this pull request May 20, 2025
Merged
1 task
bcotrim pushed a commit that referenced this pull request May 27, 2025
@epeicher @bcotrim
Sync: Update notification to include Hostname (#1412)
db6ec14 
* Push: Update notification to include Hostname

* Pull: Update notification to include hostname

* Add hints for translators
bcotrim added a commit that referenced this pull request May 27, 2025
@bcotrim
Squashed commit of the following:
6708e55 
commit d31733b
Author: katinthehatsite <[email protected]>
Date:   Tue May 27 09:12:55 2025 +0200

    Studio: Delete old proof-of-concept WP-CLI implementation (#1426)

    * Remove old scripts

    * Remove old cli.ts

    * Cleanup tests

    * Clean up terminal opening and feature flag

    * Cleanup assistant code

    * Clean shortcuts code

    * Cleanup feature flags

    * Cleanup feature flags

    * Cleanup shortcuts sessions

    * Removed unused feature flag for assistant

    * Assistant code block test fix

    * Adjust preload

    * Cleanup index file and tests

    * Refactor terminal usage

    * Add back the lock

    * Remove unintended change

    * Remove unnecesary type for warp

    * Use bundled Ids

    * Cleanup event parameter

    ---------

    Co-authored-by: Kateryna Kodonenko <[email protected]>

commit 4fac446
Author: Volodymyr Makukha <[email protected]>
Date:   Mon May 26 20:19:35 2025 +0100

    Update Ukrainian translations - May 26 (#1439)

commit c7536ef
Author: Antonio Sejas <[email protected]>
Date:   Mon May 26 18:34:28 2025 +0100

    Load LTR and RTL stylesheets conditionally (#1429)

    * Add a new component
    * Load @wordpress/components css conditionally based on RTL and LTR.
    * Load index.css a.k.a main_window.css after loading the WordPress stylesheet

commit c80b70e
Author: Bero <[email protected]>
Date:   Mon May 26 19:20:42 2025 +0200

    Update Playground packages to 1.0.38 and remove the Symlink manager (#1425)

    * Update Playground packages to 1.0.38
    * Remove SymlinkManager
    * Activate Playground's followSymlinks feature
    * Remove @php-wasm/universal patch

commit 5bfe676
Author: Wojtek Naruniec <[email protected]>
Date:   Mon May 26 15:40:18 2025 +0200

    Sanitize regex in Win editor path (#1411)

    * Sanitize regex

    * Import only escapeRegExp function

commit 2cd3e44
Author: Ivan Ottinger <[email protected]>
Date:   Mon May 26 13:18:02 2025 +0200

    Bump version from 1.5.2-beta4 to 1.5.2 (#1436)

commit f16865e
Author: Ivan Ottinger <[email protected]>
Date:   Mon May 26 12:38:24 2025 +0200

    Add release notes for 1.5.2 (#1434)

    * Add release notes for 1.5.2

    * Reorder release notes

    * Update release notes order and formatting

    * Added latest UI fix to the release notes

    * Consolidate Studio CLI fixes into a single bullet point

commit f6e48d5
Author: Ivan Ottinger <[email protected]>
Date:   Mon May 26 12:21:30 2025 +0200

    Add translations for 1.5.2 (#1433)

    * Add translations for 1.5.2

    * Add latest Polish and Spanish translations

    * Update spanish translations

    ---------

    Co-authored-by: Antonio Sejas <[email protected]>

commit ae1d163
Author: Antonio Sejas <[email protected]>
Date:   Mon May 26 10:58:22 2025 +0100

    Share same styles between preview and sync tabs (#1435)

commit d6a7905
Author: Roberto Aranda <[email protected]>
Date:   Fri May 23 15:23:14 2025 +0200

    Return original URL param when it cannot be parsed (#1420)

    * Return the passed URL parameter when it cannot be parsed instead of an empty string

commit 91d04be
Author: Antonio Sejas <[email protected]>
Date:   Fri May 23 14:22:34 2025 +0100

    Update localized docs links for Studio Sync  and Studio CLI (#1431)

    * Add translation link to new Spanish studio docs CLI
    * Add hash to Studio sync supported sites

commit 6213094
Author: katinthehatsite <[email protected]>
Date:   Fri May 23 14:27:53 2025 +0200

    Add patch to fix the accessibility with modals (#1423)

    Co-authored-by: Kateryna Kodonenko <[email protected]>

commit b2724c0
Author: Rahul Gavande <[email protected]>
Date:   Fri May 23 14:45:11 2025 +0530

    Use uri scheme to open Warp terminal app (#1428)

commit 43c831b
Author: Fredrik Rombach Ekelund <[email protected]>
Date:   Wed May 21 16:10:40 2025 +0200

    CLI: Set UTF8 encoding for output on Windows (#1424)

commit b1c0eeb
Author: Volodymyr Makukha <[email protected]>
Date:   Wed May 21 10:34:47 2025 +0100

    Fix hiding plugins spinner (#1419)

commit 48988ed
Author: Fredrik Rombach Ekelund <[email protected]>
Date:   Wed May 21 10:31:33 2025 +0200

    Add migration to rename launch uniques stat (#1415)

    * Add migration to rename launch uniques stat

    * Fix CLI zod schema

commit f363144
Author: Ivan Ottinger <[email protected]>
Date:   Tue May 20 18:24:00 2025 +0200

    Bump version to 1.5.2-beta4 (#1421)

commit 9768e67
Author: Ivan Ottinger <[email protected]>
Date:   Tue May 20 15:29:33 2025 +0200

    Pressable Sync: Fix remaining Sync modal issues (#1416)

    * Use absolute path for `WordPressLogoCircle` import

    * Use separate `isDisabled` to makde WP logo gray

    * Remove trailing space in site selector helper text

    * Decrease font size of sync sites modal learn more link

commit 9fab398
Author: Ian G. Maia <[email protected]>
Date:   Tue May 20 13:15:23 2025 +0200

    [Tooling] Update code to DRY the Windows build PS1 (#1408)

    * Update code to DRY the Windows build PS1

    * Update case of `Exit` command

    * Use `windows` instead of just `win` to prefix windows-related scripts

    * Add $LastExitCode check after `npm run make`

commit e5d4006
Author: Ivan Ottinger <[email protected]>
Date:   Tue May 20 12:01:21 2025 +0200

    Improve version comparison logic to handle prerelease transitions correctly (#1403)

commit f0a282b
Author: Rahul Gavande <[email protected]>
Date:   Tue May 20 15:15:25 2025 +0530

    HTTPS: Convert domain name Unicode characters to ASCII characters (#1400)

    * Convert domain name unicode chars to ASCII

    * Do not use punnycode domain name for paths

    * Remove unncessary punnycode domain name

commit 6cdc351
Author: Roberto Aranda <[email protected]>
Date:   Tue May 20 11:35:52 2025 +0200

    Sync: Update notification to include Hostname (#1412)

    * Push: Update notification to include Hostname

    * Pull: Update notification to include hostname

    * Add hints for translators

commit 69419a6
Author: Gergely Csécsey <[email protected]>
Date:   Tue May 20 10:30:57 2025 +0100

    Increase HTTP request timeout to 60 seconds (#1407)

    * Increase HTTP request timeout to 60 seconds

    * Increase curl timeout to 60 seconds

    * update comments

commit 6bcbbff
Author: Antonio Sejas <[email protected]>
Date:   Tue May 20 09:36:18 2025 +0100

    Update connect site text button when connecting another site (#1413)

    * Adapt the "Connect Site" button when connecting multiple sites on Sync.

commit 60a9cab
Author: Antonio Sejas <[email protected]>
Date:   Tue May 20 09:10:37 2025 +0100

    Avoid orphan in sync title (#1414)

commit 3b103cc
Author: Antonio Sejas <[email protected]>
Date:   Mon May 19 10:39:12 2025 +0100

    Release 1.5.2-beta3 (#1410)

commit a109671
Author: Ivan Ottinger <[email protected]>
Date:   Mon May 19 09:45:17 2025 +0200

    Sync: Update sync tab text and remove WordPress logo from title (#1405)

    * Update sync tab text and remove WordPress logo from title

    * Fix related test

    * Use comma

    * Update copy of the bullet points

    * Replace `WP.com` with `WordPress.com`.

    Co-authored-by: Antonio Sejas <[email protected]>

    * Replace WP.com with WordPress.com

    Co-authored-by: Antonio Sejas <[email protected]>

    * Delete unused WordPress short logo component

    ---------

    Co-authored-by: Antonio Sejas <[email protected]>

commit 716bc7a
Author: Ivan Ottinger <[email protected]>
Date:   Fri May 16 16:15:16 2025 +0200

    Sync: Update Sync modal content (#1406)

    * Update WordPress logo component `viewBox`

    * Update Sync modal heading

    * Update text below Sync search field

    * Update Sync modal site logos

    * Replace hardcoded arrows with `ArrowIcon` component in sync sites modal links

    * Localize sync modal Read more link using `getLocalizedLink` utility

commit 11b4302
Author: Roberto Aranda <[email protected]>
Date:   Fri May 16 15:14:40 2025 +0200

    Push: Read and update the progress from the sync endpoint (#1389)

    * Calls the GET /sync/import using the importId to obtain the progress
    * Use the progress from the /sync/import endpoint to update the progress in the importing state
    * Splits the progress of the importing state between backup and import

commit 7423bdf
Author: katinthehatsite <[email protected]>
Date:   Fri May 16 11:40:18 2025 +0200

    Studio: Fix no directory or file error on Windows (#1391)

    * Fix no directory or file error on Windows

    * Cleanup directories

    ---------

    Co-authored-by: Kateryna Kodonenko <[email protected]>

commit 1661ac4
Author: Antonio Sejas <[email protected]>
Date:   Fri May 16 10:04:32 2025 +0100

    Force display of what's new for minor version (#1404)

    * Force what's new display for minor version
    * Skip test that compares the patch version
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sejas sejas sejas left review comments

@gcsecsey gcsecsey gcsecsey approved these changes

@ivan-ottinger ivan-ottinger ivan-ottinger approved these changes

@katinthehatsite katinthehatsite katinthehatsite approved these changes

Assignees

@epeicher epeicher

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@epeicher @ivan-ottinger @sejas @gcsecsey @katinthehatsite