Check for version control, display notice #145

mike-day · 2023-04-20T00:45:36Z

This PR adds a dismissible notice in App if a .git folder is not found in the currently active theme directory:

Initial spike questions:

~~Should the notice be dismissible? If so, should the dismissed state persist for a given theme?~~ -> both items done
~~Should the notice should also be shown in Editor?~~ -> notice only shown in App
~~Should .svn or .hg be included in the check?~~ -> not for this version
~~What helper guide should the helper link point to?~~ -> deferred

Note about helper link deferral

While it would be a good idea to add a link to a basic guide for using git with a WordPress theme, finding a place to host that article will hold up the merging of this PR. A helper can link can be added in the future.

To-do

Make the dismissed notice state persist for a given theme
Translate VersionControlNotice strings
Add basic tests for get-version-control wp-module

How to test

Checkout the branch
Activate a theme with no .git folder in the directory
The notice should display
Dismiss the notice and refresh the page
No notice should display (the dismissed state persisted)

wp-modules/app/js/src/types.ts

mike-day · 2023-04-20T01:45:40Z

wp-modules/app/js/src/components/VersionControlNotice/index.tsx

With a light refactor to pass in versionControl and themeName as props, this component could be a good candidate for moving to common if we do something like #141.

kienstra

Hi @mike-day,
This approach is good, and it works:

I didn't comment on low-level details.

wp-modules/get-version-control/get-version-control.php

…into try/check-for-version-control

…ng context

kienstra · 2023-04-27T17:46:00Z

Cool, looking at this now!

kienstra

Hi @mike-day,
Really nice work, the notice works well, and you figured out how to test a filesystem lookup! 🚀

Some suggestions below to make this even simpler.

wp-modules/app/js/src/components/Patterns/index.tsx

wp-modules/app/js/src/components/VersionControlNotice/index.tsx

wp-modules/app/js/src/hooks/useVersionControl.ts

wp-modules/get-version-control/get-version-control.php

wp-modules/get-version-control/tests/GetVersionControlTest.php

wp-modules/api-data/api-data.php

wp-modules/app/js/src/hooks/useVersionControl.ts

wp-modules/app/app.php

kienstra · 2023-04-27T19:06:03Z

Also, maybe the message:

No version control detected for this theme. Learn how to set up git for your theme here: [Git Guide](https://wpengine.com/support/git/).

…could also mention what it means for the user, that their patterns will be deleted if they update the theme.

…tice

mike-day · 2023-04-28T15:01:37Z

Also, maybe the message:
No version control detected for this theme. Learn how to set up git for your theme here: [Git Guide](https://wpengine.com/support/git/).
…could also mention what it means for the user, that their patterns will be deleted if they update the theme.

@kienstra UX helped tweak this message for the changes applied in 1463614!

wp-modules/app/js/src/components/VersionControlNotice/index.tsx

kienstra · 2023-05-01T16:17:31Z

wp-modules/get-version-control/get-version-control.php

+ * @param string $version_control The version control directory to check.
+ * @return boolean
+ */
+function check_version_control_notice_should_show( $theme_name, $version_control = '/.git' ) {


Good idea for this, it's a very flat function.

kienstra · 2023-05-01T19:05:23Z

This is your best PR yet. Very clean functions that are well tested. And data separate from presentation in the notice component.

mike-day · 2023-05-01T19:09:53Z

Thanks for your kind words, and thanks for the help with this PR as always, @kienstra!

kienstra · 2023-05-01T19:48:33Z

Of course! Great work on this.

mike-day added 7 commits April 19, 2023 19:05

Create get-version-control wp-module

8021294

Hydrate app using check_for_git_in_theme

70ac8a1

Update global type with themename, versionControl

cbd407c

Create VersionControlNotice component

e352cc8

Remove margin-right for notice__content

c92dcae

Use VersionControlNotice in Patterns

8ca4bc5

Update AppTest

e8e2c71

mike-day commented Apr 20, 2023

View reviewed changes

wp-modules/app/js/src/types.ts Outdated Show resolved Hide resolved

Remove unused check for git in theme root

a8dbda1

mike-day commented Apr 20, 2023

View reviewed changes

mike-day added 2 commits April 20, 2023 09:56

Setup a test-theme fixture

1527f1c

Create unit tests for get-verison-control (not passing)

f3b450c

mike-day requested a review from kienstra April 20, 2023 16:11

kienstra approved these changes Apr 20, 2023

View reviewed changes

wp-modules/get-version-control/get-version-control.php Outdated Show resolved Hide resolved

mike-day added 13 commits April 20, 2023 16:18

Simplify the phpunit tests, delete unneeded fixture theme files

3e22e49

Use a param for main vc checker function so testing is easier

8031eec

Make the notice dismissible; adjust notice style to go full-width

d3f6d6b

Remove invalid max-width property

f52085a

Use info status for notice; adjust styles after talking through UX

ba16aca

Try a warning status for the notice

7379ab4

Tweak the verbiage in notice; linting for component

dbe4758

Add the update-dismissed-themes endpoint

2c7f9c0

Hydrate app with new endpoint, query for dismissedThemes

c4b39f7

Update types

7db5b54

Add a new useVersionControl hook

36641de

Call the new hook, then add to context provider

ae0d0aa

Simplify VersionControlNotice; use the updated context in Patterns

c8e0b03

mike-day changed the title ~~[SPIKE] Check for version control, display notice~~ Check for version control, display notice Apr 27, 2023

mike-day added 2 commits April 26, 2023 23:58

Update ApiDataTest

1697e26

Update AppTest

3a394e2

mike-day added 5 commits April 27, 2023 09:49

Add empty array test for get_dismissed_themes

e96e8a1

Translate the notification message

a9b9ef6

Merge branch 'main' of https://github.com/studiopress/pattern-manager …

4e8188e

…into try/check-for-version-control

Pass themeName in PatternProps

126e83f

Pass versionControl bool, handler as props in Patterns instead of usi…

06172f2

…ng context

mike-day marked this pull request as ready for review April 27, 2023 17:44

mike-day requested review from dreamwhisper and johnstonphilip April 27, 2023 17:44

mike-day requested a review from kienstra April 27, 2023 17:46

Add get_current_user_id() to setUp, tearDown in unit test

b50b045

kienstra approved these changes Apr 27, 2023

View reviewed changes

Pass Notice as an optional prop to Patterns

af0ada1

mike-day added 3 commits April 27, 2023 15:55

Simplify the logic and just hydrate with a boolean to show or hide no…

37afb1a

…tice

Bugfix from sending empty themeName; remove the POST body

54f8656

Tweak the notice message, add space

1463614

mike-day commented Apr 28, 2023

View reviewed changes

wp-modules/app/js/src/components/VersionControlNotice/index.tsx Outdated Show resolved Hide resolved

kienstra reviewed Apr 28, 2023

View reviewed changes

wp-modules/app/js/src/components/VersionControlNotice/index.tsx Outdated Show resolved Hide resolved

Add more test assertions for GetVersionControlTest

fff8336

kienstra reviewed May 1, 2023

View reviewed changes

mike-day added 2 commits May 1, 2023 11:36

Delete testing usermeta, user on tearDown

19437ed

Remove the link to Git Guide in the notice message

6c3907c

mike-day merged commit 6760041 into main May 1, 2023

mike-day deleted the try/check-for-version-control branch May 1, 2023 18:54

mike-day mentioned this pull request May 1, 2023

Cleanup for version control PR #151

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check for version control, display notice #145

Check for version control, display notice #145

mike-day commented Apr 20, 2023 •

edited

Loading

mike-day Apr 20, 2023

kienstra Apr 20, 2023

kienstra left a comment

kienstra commented Apr 27, 2023

kienstra left a comment

kienstra commented Apr 27, 2023 •

edited

Loading

mike-day commented Apr 28, 2023

kienstra May 1, 2023

kienstra commented May 1, 2023

mike-day commented May 1, 2023

kienstra commented May 1, 2023

Check for version control, display notice #145

Check for version control, display notice #145

Conversation

mike-day commented Apr 20, 2023 • edited Loading

Initial spike questions:

Note about helper link deferral

To-do

How to test

mike-day Apr 20, 2023

Choose a reason for hiding this comment

kienstra Apr 20, 2023

Choose a reason for hiding this comment

kienstra left a comment

Choose a reason for hiding this comment

kienstra commented Apr 27, 2023

kienstra left a comment

Choose a reason for hiding this comment

kienstra commented Apr 27, 2023 • edited Loading

mike-day commented Apr 28, 2023

kienstra May 1, 2023

Choose a reason for hiding this comment

kienstra commented May 1, 2023

mike-day commented May 1, 2023

kienstra commented May 1, 2023

mike-day commented Apr 20, 2023 •

edited

Loading

kienstra commented Apr 27, 2023 •

edited

Loading