Skip to content

Conversation

@dkoo
Copy link
Contributor

@dkoo dkoo commented Oct 30, 2025

All Submissions:

Changes proposed in this Pull Request:

See Automattic/newspack-scripts#219.

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully ran tests with your changes locally?

@laurelfulford
Copy link
Contributor

laurelfulford commented Oct 31, 2025

  • Run npm start or npm ci (no --legacy-peer-deps!)
  • Confirm that the install completes successfully
  • Run npm run build and npm run watch and confirm that the assets build successfully
  • Run npm run lint and confirm that SCSS and JS linting works
  • Run npm run test and confirm that JS unit tests work (only applicable if the repo has any) -- returns No JS unit tests in this repository.
  • Run npm run semantic-release --dry-run and confirm there's successful output
  • Smoke test both WP admin and front-end functionality—there should be no significant changes

@laurelfulford
Copy link
Contributor

Lint is working great 😆

src/content-distribution/content-distribution-panel/style.scss
  10:30  ✖  Unexpected whitespace after "("               @stylistic/selector-pseudo-class-parentheses-space-inside
  10:66  ✖  Unexpected whitespace before ")"              @stylistic/selector-pseudo-class-parentheses-space-inside
  15:3   ✖  Expected empty line before rule               rule-empty-line-before
  28:3   ✖  Expected empty line before rule               rule-empty-line-before
  31:3   ✖  Expected empty line before rule               rule-empty-line-before
  38:3   ✖  Expected empty line before rule               rule-empty-line-before
  41:3   ✖  Expected empty line before rule               rule-empty-line-before
  46:3   ✖  Expected empty line before rule               rule-empty-line-before
  46:9   ✖  Unexpected whitespace after "("               @stylistic/selector-pseudo-class-parentheses-space-inside
  46:24  ✖  Unexpected whitespace before ")"              @stylistic/selector-pseudo-class-parentheses-space-inside
  49:3   ✖  Expected empty line before rule               rule-empty-line-before
  49:9   ✖  Unexpected whitespace after "("               @stylistic/selector-pseudo-class-parentheses-space-inside
  49:34  ✖  Unexpected whitespace before ")"              @stylistic/selector-pseudo-class-parentheses-space-inside
  62:2   ✖  Expected empty line before rule               rule-empty-line-before
  63:3   ✖  Expected empty line before rule               rule-empty-line-before
  64:10  ✖  Expected "currentColor" to be "currentcolor"  value-keyword-case

src/content-distribution/incoming-post/style.scss
   3:2    ✖  Expected empty line before rule                                rule-empty-line-before
  23:3    ✖  Expected empty line before at-rule                             at-rule-empty-line-before
  26:4    ✖  Expected empty line before at-rule                             at-rule-empty-line-before
  30:22   ✖  Selector should use lowercase and separate words with hyphens  selector-id-pattern
  30:100  ✖  Selector should use lowercase and separate words with hyphens  selector-id-pattern
  33:4    ✖  Expected empty line before at-rule                             at-rule-empty-line-before
  36:5    ✖  Expected empty line before at-rule                             at-rule-empty-line-before
  41:104  ✖  Expected line length to be no more than 80 characters          @stylistic/max-line-length
  42:2    ✖  Selector should use lowercase and separate words with hyphens  selector-id-pattern
  43:3    ✖  Expected empty line before rule                                rule-empty-line-before
  47:28   ✖  Unexpected whitespace after "("                                @stylistic/selector-pseudo-class-parentheses-space-inside
  47:62   ✖  Unexpected whitespace before ")"                               @stylistic/selector-pseudo-class-parentheses-space-inside
  48:4    ✖  Expected empty line before at-rule                             at-rule-empty-line-before
  51:5    ✖  Expected empty line before at-rule                             at-rule-empty-line-before
  58:3    ✖  Expected empty line before at-rule                             at-rule-empty-line-before
  61:4    ✖  Expected empty line before at-rule                             at-rule-empty-line-before
  65:84   ✖  Expected line length to be no more than 80 characters          @stylistic/max-line-length
  67:3    ✖  Expected empty line before at-rule                             at-rule-empty-line-before
  70:4    ✖  Expected empty line before at-rule                             at-rule-empty-line-before

Copy link
Contributor

@laurelfulford laurelfulford left a comment

Choose a reason for hiding this comment

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

When I got to edit posts on this PR, I get this error on both the Hub and Network; I can't recreate it on trunk:

CleanShot 2025-10-31 at 13 05 18

In the inspector, it comes up as Uncaught TypeError: _isSavingPost is not a function.

I didn't run into errors on the other screens - just let me know if you can't recreate this!

Edited to add: I suspect the same thing would happen with _isCleanNewPost since they're formatted the same in the PR, but I didn't actually see the same error for that one!

@dkoo
Copy link
Contributor Author

dkoo commented Oct 31, 2025

@laurelfulford whoops, it looks like this repo was lacking a .stylelintrc.js file because it only recently started containing any SCSS. I've added that and ran the npm run format:scss to fix most of the linting errors.

I also saw that there's some redundancy in the .stylelintrc.js files across our repos. I moved the ignoreFiles rules from each individual repo's config to newspack-scripts/config/stylelint.config.js. Some repos were also missing a .stylelintrc.js so I added one where needed.

You may need to remove node_modules and run npm ci again before lint:scss will pass, but you can also just check whether the job passes below 👇

@dkoo dkoo requested a review from laurelfulford October 31, 2025 21:33
@laurelfulford
Copy link
Contributor

Thanks @dkoo! I'm still getting a couple lint errors locally (weird though, your commit is happy!):

release/newspack-network/src/content-distribution/content-distribution-panel/style.scss
  64:10  ✖  Expected "currentColor" to be "currentcolor"  value-keyword-case

release/newspack-network/src/content-distribution/incoming-post/style.scss
  30:22   ✖  Selector should use lowercase and separate words with hyphens  selector-id-pattern
  30:100  ✖  Selector should use lowercase and separate words with hyphens  selector-id-pattern
  42:2    ✖  Selector should use lowercase and separate words with hyphens  selector-id-pattern

I think as long as the tests are passing in GitHub we could tackle these as a separate PR 🙂 I think the main remaining issue is that Uncaught TypeError: _isSavingPost is not a function. error.

@dkoo
Copy link
Contributor Author

dkoo commented Oct 31, 2025

Whoops, missed that comment. 10201a2 should fix the JS error!

Copy link
Contributor

@laurelfulford laurelfulford left a comment

Choose a reason for hiding this comment

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

Thanks @dkoo! That took care of the error 🎉

@dkoo dkoo marked this pull request as ready for review November 3, 2025 18:29
@dkoo dkoo requested a review from a team as a code owner November 3, 2025 18:29
@dkoo dkoo merged commit bb89be6 into trunk Nov 3, 2025
5 checks passed
@dkoo dkoo deleted the chore/update-dependencies-oct-2025 branch November 3, 2025 18:30
@github-actions
Copy link

github-actions bot commented Nov 3, 2025

Hey @dkoo, good job getting this PR merged! 🎉

Now, the needs-changelog label has been added to it.

Please check if this PR needs to be included in the "Upcoming Changes" and "Release Notes" doc. If it doesn't, simply remove the label.

If it does, please add an entry to our shared document, with screenshots and testing instructions if applicable, then remove the label.

Thank you! ❤️

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.

3 participants