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

T13680 breadcrumbs component #13682

Merged
merged 9 commits into from
Dec 24, 2018
Merged

T13680 breadcrumbs component #13682

merged 9 commits into from
Dec 24, 2018

Conversation

niden
Copy link
Member

@niden niden commented Dec 23, 2018

Hello!

In raising this pull request, I confirm the following (please check boxes):

  • I have read and understood the Contributing Guidelines?
  • I have checked that another pull request for this purpose does not exist.
  • I wrote some tests for this PR.

Added `Html\Breadcrumbs component, offering an easy way to create breadcrumbs for your application

Thanks

@niden niden added the enhancement Enhancement to the framework label Dec 23, 2018
@niden niden requested a review from sergeyklay December 23, 2018 02:41
@niden
Copy link
Member Author

niden commented Dec 23, 2018

cc @sergeyklay

* upstream/4.0.x:
  Merge for #12547
  Added some methods to Phalcon\Acl\Adapter\Memory
  [#12295] - Removed the final from the class definition
  [#12295] - Updated the changelog
  [#12295] - Added container tests
  [#12295] - Added Container object, PSR-11 compliant
  [#12295] - Added entry to the changelog
  [#12295] - Fixed interface
  [4.0.x] - Another correction to the test
* 4.0.x:
  Merge for #12547
  Added some methods to Phalcon\Acl\Adapter\Memory
  [#12295] - Removed the final from the class definition
  [#12295] - Updated the changelog
  [#12295] - Added container tests
  [#12295] - Added Container object, PSR-11 compliant
  [#12295] - Added entry to the changelog
  [#12295] - Fixed interface
  [4.0.x] - Another correction to the test
Copy link
Contributor

@CameronHall CameronHall left a comment

Choose a reason for hiding this comment

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

Could you please fix up the two minor things I found in the docblocks? Thank you!

* Clears the crumbx
*
* <code>
* $$breadcrumbs->clear()
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo here with the extra $

/**
* Removes crumb by url.
*
* <core>
Copy link
Contributor

Choose a reason for hiding this comment

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

<core> should be <code>

* upstream/4.0.x:
  Fixes #12975, #13477: allowEmptyStrings & skipAttributes adhere col map
* 4.0.x:
  Fixes #12975, #13477: allowEmptyStrings & skipAttributes adhere col map
Copy link
Contributor

@sergeyklay sergeyklay left a comment

Choose a reason for hiding this comment

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

There are some minor issues mentioned by @CameronHall, but on the whole LGTM

@niden niden merged commit 7fd044b into phalcon:4.0.x Dec 24, 2018
@niden
Copy link
Member Author

niden commented Dec 24, 2018

@sergeyklay Already addressed them

@niden niden deleted the T13680-breadcrumbs-component branch January 21, 2019 19:18
@niden niden added the documentation Documentation required label Apr 9, 2019
@niden niden added 4.0 and removed documentation Documentation required labels Oct 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement to the framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants