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

Feature/frontpage views #32

Merged
merged 9 commits into from
Apr 20, 2021
Merged

Feature/frontpage views #32

merged 9 commits into from
Apr 20, 2021

Conversation

martinyde
Copy link

https://jira.itkdev.dk/browse/LOOP-735

  • Adds new module for lists

@martinyde martinyde requested a review from rimi-itk April 19, 2021 21:00
Copy link

@rimi-itk rimi-itk left a comment

Choose a reason for hiding this comment

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

Looks good. Some minor changes needed.

@@ -3,23 +3,26 @@ langcode: en
status: true
dependencies:
config:
- field.field.paragraph.os2loop_section_page_views_refer.os2loop_section_page_view
- field.field.paragraph.os2loop_section_page_views_refer.field_os2loop_section_page_block

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

Changed

id: paragraph.os2loop_section_page_views_refer.default
targetEntityType: paragraph
bundle: os2loop_section_page_views_refer
mode: default
content:
os2loop_section_page_view:
weight: 2
field_os2loop_section_page_block:

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

Changed

Comment on lines 111 to 113
foreach ($expertises as $expertise) {
$term_ids[] = $expertise['target_id'];
}

Choose a reason for hiding this comment

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

You could use array_column($expertises, 'target_id') here to do the same.

Copy link
Author

Choose a reason for hiding this comment

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

Changed

Copy link
Author

Choose a reason for hiding this comment

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

Changed

Comment on lines 67 to 75
return $this->entityTypeManager
->getListBuilder('node')
->getStorage()
->loadByProperties([
'type' => 'os2loop_question',
'status' => 1,
'os2loop_shared_subject' => $user_expertises,
]);
}

Choose a reason for hiding this comment

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

You have almost the same code in both getContentByUserExpertise and getContentByUserProfession. Consider moving it into a helper function that can be called (twice).

Copy link
Author

Choose a reason for hiding this comment

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

Added helper function

<div class="list-group">
<article class="list-group-item mb-2 bg-light border-0">
<div class="text-muted small">{{ node.type.entity.label }} </div>
<h2 class="h6"><a href="{{ url('entity.node.canonical', { 'node': node.id() }) }}">{{ node.title() }}</a></h2>

Choose a reason for hiding this comment

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

h2 element with class name h6? Is that a typo?

Copy link
Author

Choose a reason for hiding this comment

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

Hmm. This should be h3.
H3 is the correct html tag. H6 is a bootstrap helper class to resize the header, instead of styling it.

Copy link
Author

Choose a reason for hiding this comment

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

CHanged to h3

*/
#}
{% for item in items %}
<h2{{ attributes.addClass("h4") }}>{{ item.content }}</h2>

Choose a reason for hiding this comment

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

h2 vs h4. Am i missing something?

Copy link
Author

Choose a reason for hiding this comment

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

H2 is the correct html tag. H4 is a bootstrap helper class to resize the header, instead of styling it.

@martinyde martinyde requested a review from rimi-itk April 20, 2021 08:36
@martinyde martinyde merged commit 951dfcc into develop Apr 20, 2021
@martinyde martinyde deleted the feature/frontpage-views branch April 20, 2021 08:59
rimi-itk added a commit that referenced this pull request Apr 20, 2021
…-external-source_03_2021

Feature/loop 751 add template for external source 03 2021
rimi-itk pushed a commit that referenced this pull request Apr 20, 2021
rimi-itk pushed a commit that referenced this pull request Aug 12, 2022
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.

2 participants