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

add missing env informations to block #401 #403

Merged
merged 2 commits into from
Feb 5, 2024
Merged

Conversation

nadar
Copy link
Member

@nadar nadar commented Jan 25, 2024

closes #401

Copy link
Member

@hbugdoll hbugdoll left a comment

Choose a reason for hiding this comment

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

For env options 'index', 'itemsCount', 'isFirst', 'isLast', 'isPrevEqual', 'isNextEqual' and 'equalIndex' it's fine now.
But for env option 'pageObject'

$blockObject = $blockItem->block->getObject($blockItem->id, 'admin', $navItemPage);
is still wrong – NavItem instead of NavItemPage is needed as third argument.

@hbugdoll
Copy link
Member

hbugdoll commented Feb 5, 2024

I've made new unit tests for the envOptions: ef4efcd.

And they have proven their worth – my 'equalIndex' iteration was wrong (post-increment vs. pre-increment), see 2d3be0d.

Block Test

  • Added new BlockTest::testBlockEnvOptions()
  • Enhanced therefore TestBlock::admin()
    • Accessing and returning envOptions and unused config value
  • Improved BlockTest::testBlockValues() (checking unused config value)

Block Placeholder Iteration Test

  • Added new test BlockPlaceholderIterationTest::testEnvOptionsPlaceholderIteration()
    • 3 equal blocks on 1 page via fixtures
    • Checking envOptions in admin context via NavItemPage::getPlaceholder()
    • Checking envOptions in frontend context via NavItemPage::renderPlaceholder()
  • Added therefore new testing block TestingEnvOptionsBlock
    • Accessing on envOptions in admin context inside renderAdmin()
    • Accessing on envOptions in frontend context inside renderFrontend() (JSON decoding/encoding is needed because of internal processing & concat)
  • Completed access on all block models in BlockPlaceholderIterationTest::testRenderPlaceholderIteration() for sake of completeness

@nadar
Copy link
Member Author

nadar commented Feb 5, 2024

@hbugdoll i will merge this PR.. You can send then a PR with only the unit tests and the according fix. ok? Afterwards we will take a look at the page object in another PR.

@nadar nadar merged commit e8afe25 into master Feb 5, 2024
14 of 15 checks passed
@nadar nadar deleted the block-admin-env-option branch February 5, 2024 13:45
@nadar
Copy link
Member Author

nadar commented Feb 5, 2024

@hbugdoll you can now create a fresh merge request using the latest master (don't forget to create a branch on your fork).

@hbugdoll
Copy link
Member

hbugdoll commented Feb 5, 2024

@hbugdoll you can now create a fresh merge request using the latest master (don't forget to create a branch on your fork).

Ok, done → #407

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.

Some block env options are missing in admin context
2 participants