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

FIX / DOC Add support ElementalArea in DataObject #978

Conversation

sabina-talipova
Copy link
Contributor

@sabina-talipova sabina-talipova commented Apr 21, 2022

Description

Create new section in docs directory with example how to use ElementalArea together with DataObject.

Changes

New condition was added in CMSEditLink() in BaseElement class for cause when $page is not an instance of SiteTree and Element is not inline editable. Also new condition for checking of existing the CMSEditLink() method in class that has relation with ElemantalArea.

Parent Issue

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.

I wasted able to get the CMS preview to work. Using the debugger, cmsPreview() nor PreviewLink were getting called. I followed the instructions inthe document I'll paste my code below.

docs/en/examples/using_elementalArea_with_dataObject.md Outdated Show resolved Hide resolved
docs/en/examples/using_elementalArea_with_dataObject.md Outdated Show resolved Hide resolved
docs/en/examples/using_elementalArea_with_dataObject.md Outdated Show resolved Hide resolved
docs/en/examples/using_elementalArea_with_dataObject.md Outdated Show resolved Hide resolved
docs/en/examples/using_elementalArea_with_dataObject.md Outdated Show resolved Hide resolved
docs/en/examples/using_elementalArea_with_dataObject.md Outdated Show resolved Hide resolved
docs/en/examples/using_elementalArea_with_dataObject.md Outdated Show resolved Hide resolved
docs/en/examples/using_elementalArea_with_dataObject.md Outdated Show resolved Hide resolved
docs/en/examples/using_elementalArea_with_dataObject.md Outdated Show resolved Hide resolved
docs/en/examples/using_elementalArea_with_dataObject.md Outdated Show resolved Hide resolved
@emteknetnz
Copy link
Member

Also could you rename using_elementalArea_with_dataObject.md to be lower case using_elementalarea_with_dataobject.md - you'll probably need to follow these instructions

@sabina-talipova sabina-talipova force-pushed the pulls/4.8/dataobject-in-elemental branch from 5f92a13 to cbced82 Compare April 25, 2022 22:34
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 these docs are duplication of what's already on framework - https://docs.silverstripe.org/en/4/developer_guides/customising_the_admin_interface/preview/ / https://github.com/silverstripe/silverstripe-framework/blob/4/docs/en/02_Developer_Guides/15_Customising_the_Admin_Interface/04_Preview.md

We should aim to not have duplicated docs if possible - would it be possible to just remove the duplicated bits and link to the framework docs?

docs/en/examples/elementalarea_with_dataobject.md Outdated Show resolved Hide resolved
docs/en/examples/elementalarea_with_dataobject.md Outdated Show resolved Hide resolved
src/Models/BaseElement.php Outdated Show resolved Hide resolved
src/Models/BaseElement.php Outdated Show resolved Hide resolved
src/Models/BaseElement.php Outdated Show resolved Hide resolved
@sabina-talipova sabina-talipova force-pushed the pulls/4.8/dataobject-in-elemental branch from cbced82 to 4c74c46 Compare April 26, 2022 23:02
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 a lot better now that the Previewable examples have been removed.

I've followed the example code locally, the BlogPost's aren't publishable, which is unexpected given the content blocks are inline publishable

docs/en/examples/elementalarea_with_dataobject.md Outdated Show resolved Hide resolved
@sabina-talipova sabina-talipova force-pushed the pulls/4.8/dataobject-in-elemental branch from 1ea26f7 to 693a6d9 Compare April 28, 2022 23:08
src/Models/BaseElement.php Outdated Show resolved Hide resolved
src/Models/BaseElement.php Outdated Show resolved Hide resolved
src/Models/BaseElement.php Outdated Show resolved Hide resolved
src/Models/BaseElement.php Outdated Show resolved Hide resolved
src/Models/BaseElement.php Outdated Show resolved Hide resolved
src/Models/BaseElement.php Outdated Show resolved Hide resolved
src/Models/BaseElement.php Outdated Show resolved Hide resolved
src/Models/BaseElement.php Outdated Show resolved Hide resolved
src/Models/BaseElement.php Show resolved Hide resolved
src/Models/BaseElement.php Outdated Show resolved Hide resolved
@sabina-talipova sabina-talipova force-pushed the pulls/4.8/dataobject-in-elemental branch 4 times, most recently from 0d5f0be to e6809e3 Compare May 2, 2022 21:39
Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

I haven't actually tried running the example locally yet, but I figured I may as well submit this much of the review while I do that.

There are some questions here that you may have already gone through with Steve - if so, sorry about the duplication. Just want to make sure I fully understand what is happening here.

docs/en/examples/elementalarea_with_dataobject.md Outdated Show resolved Hide resolved
docs/en/examples/elementalarea_with_dataobject.md Outdated Show resolved Hide resolved
docs/en/examples/elementalarea_with_dataobject.md Outdated Show resolved Hide resolved
docs/en/examples/elementalarea_with_dataobject.md Outdated Show resolved Hide resolved
docs/en/examples/elementalarea_with_dataobject.md Outdated Show resolved Hide resolved
docs/en/examples/elementalarea_with_dataobject.md Outdated Show resolved Hide resolved
src/Models/BaseElement.php Outdated Show resolved Hide resolved
src/Models/BaseElement.php Outdated Show resolved Hide resolved
tests/BaseElementTest.php Outdated Show resolved Hide resolved
tests/BaseElementTest.php Outdated Show resolved Hide resolved
@sabina-talipova sabina-talipova force-pushed the pulls/4.8/dataobject-in-elemental branch 2 times, most recently from c4cb739 to 28c171e Compare May 4, 2022 04:31
Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

Nice work!
There are a few minor tweaks to make on the documentation, and some code quality tweaks to make, but I think it's just about ready.
I haven't done so in my review, but once you've made these changes can you please make sure to run phpcs? (You probably already do this but just in case)

At some stage we'll want behat coverage to ensure elemental blocks continue to work in arbitrary DataObjects in the future... I'll let you decide whether you want to do that with this PR or create an issue for it to be added later, given the time you've already spent on this one.

After thinking about this I'm taking this decision for you :p just creat an issue, it should be a separate piece of work. Adding behat tests for this will be a big piece of work.

docs/en/examples/elementalarea_with_dataobject.md Outdated Show resolved Hide resolved
tests/BaseElementTest.php Outdated Show resolved Hide resolved
docs/en/examples/elementalarea_with_dataobject.md Outdated Show resolved Hide resolved
src/Models/BaseElement.php Outdated Show resolved Hide resolved
src/Models/BaseElement.php Outdated Show resolved Hide resolved
src/Models/BaseElement.php Outdated Show resolved Hide resolved
src/Models/BaseElement.php Outdated Show resolved Hide resolved
src/Models/BaseElement.php Outdated Show resolved Hide resolved
@sabina-talipova
Copy link
Contributor Author

sabina-talipova commented May 4, 2022

Regarding "After thinking about this I'm taking this decision for you :p just creat an issue, it should be a separate piece of work. Adding behat tests for this will be a big piece of work."
I think, it would be better to implement behat tests together with this task, then leave it for long time. Probably, test cases for Elements on a Page already exist, I just need to add tests for DataObject. Am I right?

I also want to add test cases for canDelete, Link and AbsoluteLink methods.

@GuySartorelli
Copy link
Member

I just need to add tests for DataObject. Am I right?

You'd also need to add fixtures for it, which can't be done easily as TestOnly objects in behat, so it would mean adding new fixtures to https://github.com/silverstripe/silverstripe-frameworktest and adding that repository as a dev dependency for elemental.
I won't stop you from adding that to this PR if that's what you'd prefer to do, but if you get frustrated with it I'd also be okay merging this without behat tests.

I also want to add test cases for canDelete, Link and AbsoluteLink methods.

That sounds like a good idea

@sabina-talipova sabina-talipova force-pushed the pulls/4.8/dataobject-in-elemental branch 5 times, most recently from 590ad9e to b488f38 Compare May 6, 2022 01:41
Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

Tests are failing. Ignore the code coverage one for now - there's not really anything you can do about that. I'm hoping it will resolve itself by the time you push a fix for the other failing tests.

tests/Src/TestDataObjectWithCMSEditLink.php Outdated Show resolved Hide resolved
@sabina-talipova sabina-talipova force-pushed the pulls/4.8/dataobject-in-elemental branch 2 times, most recently from 6258888 to 18a09ae Compare May 8, 2022 23:32
@GuySartorelli
Copy link
Member

There's a note above to address, plus some changes requested in silverstripe/silverstripe-frameworktest#105 - once those are addressed I'll test the behat tests locally

@sabina-talipova sabina-talipova force-pushed the pulls/4.8/dataobject-in-elemental branch from 18a09ae to 38a50b0 Compare May 9, 2022 04:36
Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

Sorry, should have noticed this in the last review.
Looks good now, I'll check that the behat tests run with the fixtures in frameworktest when I have a moment. If that passes fine, I'll approve the fixtures and then get travis to run the tests again here before merging.

@sabina-talipova sabina-talipova force-pushed the pulls/4.8/dataobject-in-elemental branch from 38a50b0 to e9cf576 Compare May 9, 2022 04:52
@GuySartorelli
Copy link
Member

Behat tests work locally. Restarting travis tests now that the fixtures are merged.

@GuySartorelli GuySartorelli force-pushed the pulls/4.8/dataobject-in-elemental branch from e9cf576 to d867213 Compare May 10, 2022 22:07
Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

Tests are failing that aren't failing on the 4.8 branch

@sabina-talipova sabina-talipova force-pushed the pulls/4.8/dataobject-in-elemental branch 2 times, most recently from ae97360 to 1306ae4 Compare May 11, 2022 04:18
@sabina-talipova sabina-talipova force-pushed the pulls/4.8/dataobject-in-elemental branch from 1306ae4 to d117559 Compare May 11, 2022 04:40
Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

Tested locally, works as expected.
The only remaining failures are also observed on the 4.8 branch, and are unrelated to this PR.

@GuySartorelli GuySartorelli merged commit b18c209 into silverstripe:4.8 May 11, 2022
@GuySartorelli GuySartorelli deleted the pulls/4.8/dataobject-in-elemental branch May 11, 2022 21:15
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.

3 participants