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.

Sign up for GitHub

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

Jump to bottom

add InternalExternalLink form type #13

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

add InternalExternalLink form type #13

wants to merge 1 commit into from

Conversation

bdejacobet
Copy link
Collaborator

@bdejacobet bdejacobet commented Mar 21, 2018 •
edited
Loading

I am targeting this branch, because it is a new feature

Changelog

### Added
- InternalExternalLink form type.

### Changed
- Must add sonata page bundle to your project

To do

  • Add sonata-project/page-bundle to dependencies
  • Add InternalExternalLink form type with all needed elements (js, css, twig, ...)
  • Add validator for InternalExternalLink form type
  • Update the documentation
  • Add tests
  • Add Changelog

Subject

Add InternalExternalLink form type

internal-external-link-choices

internal-external-link-external

internal-external-link-internal-params

internal-external-link-internal-closed

@bdejacobet bdejacobet self-assigned this Mar 21, 2018
mazsudo
mazsudo suggested changes Mar 21, 2018
View reviewed changes
composer.lock Outdated
@@ -0,0 +1,6601 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

this file should not be commited ;)

docs/20-internal-external-form-type.md Outdated
## How it works

InternalExternalLink form type groups many fields to record each type of values.
Javascript is use to display form and manage interaction between each field.
Copy link
Contributor

Choose a reason for hiding this comment

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

used

src/DependencyInjection/SonataHelperExtension.php Outdated
* file that was distributed with this source code.
*
*/
namespace Sonata\SonataHelpersBundle\DependencyInjection;
Copy link
Contributor

Choose a reason for hiding this comment

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

add blank line above plz ;)

src/DependencyInjection/SonataHelperExtension.php Outdated
*
*/
namespace Sonata\SonataHelpersBundle\DependencyInjection;
use Symfony\Component\Config\FileLocator;
Copy link
Contributor

Choose a reason for hiding this comment

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

add blank line above plz ;)

src/DependencyInjection/SonataHelperExtension.php Outdated
use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\DependencyInjection\Loader\XmlFileLoader;
use Symfony\Component\HttpKernel\DependencyInjection\Extension;
/**
Copy link
Contributor

Choose a reason for hiding this comment

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

add blank line above plz ;)

src/Entity/PageManager.php
->select('page')
->from(Page::class, 'page')
->andWhere('page.requestMethod LIKE :request_method', 'page.routeName NOT LIKE :fos_user')
->andWhere('page.routeName NOT LIKE :sonata_cache', 'page.url IS NOT NULL')
Copy link
Contributor

Choose a reason for hiding this comment

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

nice exclusions ;)
maybe we should add : ->andWhere('page.routeName NOT LIKE :internal_page')
and 'internal_page' => '_page_internal%', in parameters

Copy link
Contributor

Choose a reason for hiding this comment

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

agree. It should use the same filters as the CMS router.
It could be even more handy to get routes directly from the router

@mazsudo
Copy link
Contributor

mazsudo commented Mar 21, 2018

thx @bdejacobet .

Could we imagine in the future to link it with ArticleBundle to also be able to choose a link to an article? ;)

srascar
srascar suggested changes Mar 21, 2018
View reviewed changes
src/Entity/PageManager.php
->select('page')
->from(Page::class, 'page')
->andWhere('page.requestMethod LIKE :request_method', 'page.routeName NOT LIKE :fos_user')
->andWhere('page.routeName NOT LIKE :sonata_cache', 'page.url IS NOT NULL')
Copy link
Contributor

Choose a reason for hiding this comment

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

agree. It should use the same filters as the CMS router.
It could be even more handy to get routes directly from the router

src/Entity/PageManager.php Outdated
])
;

if (null !== $site) {
Copy link
Contributor

Choose a reason for hiding this comment

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

$site can't be null since the function definition doesn't allow it

src/Form/DataTransformer/PageDataTransformer.php
*
* @author Benoit de Jacobet <[email protected]>
*/
class PageDataTransformer implements DataTransformerInterface
Copy link
Contributor

Choose a reason for hiding this comment

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

This class has the same behaviour as a normalizer.
It should be integrated using the serializer component of Symfony.
This way we could use context to handle different behaviours
instead of an internal $keysToTransform property

src/Validator/Constraints/InternalExternalLinkValidator.php
case 'link':
if (!$values['link']) {
$this->context->buildViolation($constraint->message)
->setParameter('{{ error }}', 'Il faut saisir une url externe')
Copy link
Contributor

Choose a reason for hiding this comment

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

FR text

@bdejacobet bdejacobet force-pushed the internal-external-link branch from cd3d874 to 82033d2 Compare March 21, 2018 16:19
@bdejacobet bdejacobet mentioned this pull request Apr 8, 2019
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mazsudo mazsudo mazsudo requested changes

@srascar srascar srascar requested changes

At least 1 approving review is required to merge this pull request.

Assignees

@bdejacobet bdejacobet

Labels
feature in progress
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bdejacobet @mazsudo @srascar