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

MNT Add tests for multi-WYSIWYG config regressions #1135

Conversation

GuySartorelli
Copy link
Member

Description

Tests for regressions for having multiple WYSIWYG configs.
Tests are for silverstripe/silverstripe-admin#1512 and #1120

Needs all three of the below PRs to be merged for the CI to go green:

Manual testing steps

Load up an environment with the three linked PRs included, and run the new behat test.

Issues

Pull request checklist

  • The target branch is correct
  • All commits are relevant to the purpose of the PR (e.g. no debug statements, unrelated refactoring, or arbitrary linting)
    • Small amounts of additional linting are usually okay, but if it makes it hard to concentrate on the relevant changes, ask for the unrelated changes to be reverted, and submitted as a separate PR.
  • The commit messages follow our commit message guidelines
  • The PR follows our contribution guidelines
  • Code changes follow our coding conventions
  • This change is covered with tests (or tests aren't necessary for this change)
  • CI is green

use SilverStripe\Forms\HTMLEditor\HTMLEditorField;

/**
* @extends Extension<BaseElement>
Copy link
Member Author

Choose a reason for hiding this comment

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

This adds no benefit but does no harm in 4.x.
In 5.x it tells your IDE what $this->owner and $this->getOwner() are.

@GuySartorelli GuySartorelli force-pushed the pulls/4.11/behat-for-tinymcelink branch from 97166fb to f58b935 Compare January 24, 2024 02:36
Copy link
Member

@emteknetnz emteknetnz left a comment

Choose a reason for hiding this comment

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

Seems like this should be in admin, not elemental?

@GuySartorelli
Copy link
Member Author

GuySartorelli commented Jan 24, 2024

Relies on elemental blocks - has to be in elemental.

Update: I have tried to reproduce this without elemental and haven't been able to do so.

Copy link
Member

@emteknetnz emteknetnz left a comment

Choose a reason for hiding this comment

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

Code is fine, CI is red

@GuySartorelli
Copy link
Member Author

Rerunning CI - as per PR description it needed the others merged first before it could go green.

@emteknetnz emteknetnz merged commit 6958e89 into silverstripe:4.11 Jan 25, 2024
12 checks passed
@emteknetnz emteknetnz deleted the pulls/4.11/behat-for-tinymcelink branch January 25, 2024 02:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants