Add list of content to the homepage by nlisgo · Pull Request #64 · libero/browser

nlisgo · 2019-04-02T20:07:11Z

No description provided.

…d—homepage-lazy

vendor-extra/LiberoPageBundle/src/EventListener/BuildView/ItemListListener.php

vendor-extra/JatsContentBundle/src/EventListener/BuildView/ItemTeaserListener.php

nlisgo · 2019-04-03T09:03:43Z

Since 84a4228 I am experiencing an issue with the heading variable not being available in the teaser template. I suspect the href is not available either. Not sure why this small adjustment is causing the teaser template not to work.

vendor-extra/LiberoPageBundle/tests/TwigTestCase.php

giorgiosironi · 2019-04-09T11:40:55Z

config/packages/dev/libero_page.yaml

    pages:
        homepage:
            path: '/'
+            primary_listing: 'scholarly-articles/items'


seems flexible enough to allow the different services, does it need additions to sample-configuration then?

browser/vendor-extra/LiberoPageBundle/src/DependencyInjection/LiberoPageConfiguration.php

Lines 64 to 66 in 3bb1391

->scalarNode('primary_listing')

->isRequired()

->end()

Is required, but can point at either content or search API (anything that returns a libero:item-list).

giorgiosironi · 2019-04-09T11:52:25Z

...xtra/JatsContentBundle/tests/EventListener/BuildView/FrontArticleTitleTeaserListenerTest.php

+        $listener->onBuildView($event);
+        $view = $event->getView();
+
+        $this->assertInstanceOf(TemplateView::class, $view);


I understand the negative cases but what would be a way of shortening the assertion phases of these tests? Extract a custom assertion method that contains this phase seems cheap and effective.

this->assertEquals(new TemplateView('template'), $view); works.

Exists in other tests too, will follow up with a refactor.

...xtra/JatsContentBundle/tests/EventListener/BuildView/FrontArticleTitleTeaserListenerTest.php

giorgiosironi · 2019-04-09T11:56:11Z

vendor-extra/LiberoContentBundle/tests/EventListener/BuildView/FrontTitleTeaserListenerTest.php

+
+        $element = $this->loadElement(
+            <<<XML
+<libero:front xmlns:libero="http://libero.pub">


a pity that this is a completely duplicated test in another bundle, that just changes the input from JATS to libero XML.

Might be able to do something to help these different implementations and their test suites, but definitely one for the future.

giorgiosironi · 2019-04-09T11:57:47Z

vendor-extra/LiberoPageBundle/src/EventListener/BuildView/ItemListListener.php

+
+        $level = $view->getContext()['level'] ?? 1;
+        if ($view->hasContext('list_title')) {
+            $level++;


mutable counter in a long method, tricky

Think can break this listener up.

vendor-extra/LiberoPageBundle/src/EventListener/BuildView/ItemListListener.php

giorgiosironi · 2019-04-09T12:06:19Z

vendor-extra/LiberoPageBundle/src/EventListener/BuildView/ItemRefTeaserHrefListener.php

+
+        return $view->withArgument(
+            'href',
+            $this->urlGenerator->generate("libero.page.content.{$service}", ['id' => $id])


good abstraction of UrlGenerator as an application service that is passed in where needed

giorgiosironi · 2019-04-09T12:08:48Z

vendor-extra/LiberoPageBundle/src/EventListener/BuildView/ItemRefTeaserListener.php

+    private $client;
+    private $converter;
+
+    public function __construct(ClientInterface $client, ViewConverter $converter)


mixing the Listeners with Clients is the dangerous part in terms of layering, but I'd agree there's no other clear option here at the moment

thewilkybarkid and others added 24 commits March 19, 2019 11:05

Add homepage

2be2d28

Not there anymore

2350dfc

Update existing tests

75b044d

Use specific exception

ab67c94

Merge branch 'master' into homepage

17a3b37

Merge fixes

2a92d14

Set HTML title using translations

ace03ab

Add homepage to integration tests

3f35854

Fix and test title

087a021

Merge branch 'master' into homepage

24045d5

Extract PageEvent

7222c0c

Merge branch 'master' into homepage

4ebe45f

Missed some cases

9b8a204

Merge branch 'master' into homepage

572d5c3

Return type

737ff95

Tweak configuration

9880898

Improve name

1bc13dc

Changes

f125fbb

Merge branch 'master' into homepage-lazy

9c244af

Merge branch 'master' of github.com:libero/browser into thewilkybarki…

06b0ce8

…d—homepage-lazy

Introduce pattern library teaser templates

e658709

Introduce teaser pattern

ee06b3f

Note to set route to content

95516a4

Adjust arguments handled

d8a1bb2

nlisgo requested a review from a team as a code owner April 2, 2019 20:07

nlisgo commented Apr 2, 2019

View reviewed changes

vendor-extra/LiberoPageBundle/src/EventListener/BuildView/ItemListListener.php Outdated Show resolved Hide resolved

nlisgo commented Apr 2, 2019

View reviewed changes

vendor-extra/JatsContentBundle/src/EventListener/BuildView/ItemTeaserListener.php Outdated Show resolved Hide resolved

thewilkybarkid mentioned this pull request Apr 3, 2019

Separate View into different implementations #63

Merged

Use teaser-list template

84a4228

Add tests

922cf53

thewilkybarkid reviewed Apr 9, 2019

View reviewed changes

vendor-extra/LiberoPageBundle/tests/TwigTestCase.php Show resolved Hide resolved

Update config

30d382d

thewilkybarkid changed the title ~~[WIP] Add list of articles to the homepage~~ [WIP] Add list of content to the homepage Apr 9, 2019

Tidy

f3b6e6c

thewilkybarkid changed the title ~~[WIP] Add list of content to the homepage~~ Add list of content to the homepage Apr 9, 2019

thewilkybarkid and others added 8 commits April 9, 2019 09:02

Not needed here

7c2310e

Add title

0cdb625

Merge branch 'master' into homepage-lazy—teaser

b7ee7af

Set levels

5868bdb

Put in grid and fix levels

39edf12

Set empty text

61e5820

Update

10177c5

Update

3bb1391

thewilkybarkid approved these changes Apr 9, 2019

View reviewed changes

giorgiosironi reviewed Apr 9, 2019

View reviewed changes

...xtra/JatsContentBundle/tests/EventListener/BuildView/FrontArticleTitleTeaserListenerTest.php Outdated Show resolved Hide resolved

giorgiosironi reviewed Apr 9, 2019

View reviewed changes

Fix terminology

56ec635

giorgiosironi reviewed Apr 9, 2019

View reviewed changes

vendor-extra/LiberoPageBundle/src/EventListener/BuildView/ItemListListener.php Outdated Show resolved Hide resolved

giorgiosironi reviewed Apr 9, 2019

View reviewed changes

thewilkybarkid added 3 commits April 9, 2019 15:11

Simplify

c3d74a4

Split into three

63f41a0

Missing assertion

9605f3e

giorgiosironi approved these changes Apr 9, 2019

View reviewed changes

thewilkybarkid merged commit 5bfb50b into libero:master Apr 9, 2019

thewilkybarkid mentioned this pull request Apr 10, 2019

Fix return type in SectionListener #70

Merged

Conversation

nlisgo commented Apr 2, 2019

Uh oh!

Uh oh!

Uh oh!

nlisgo commented Apr 3, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thewilkybarkid Apr 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

thewilkybarkid Apr 9, 2019 •

edited

Loading