Skip to content

Conversation

@martinyde
Copy link

  • Messages view update
  • Message languages fix
  • Cleanup templates
  • Modify icon asstes

@martinyde martinyde requested a review from rimi-itk May 5, 2021 12:55
$dark-upvote: darken($primary-color, 10%);
$dark-follow-content: darken($primary-color, 10%);
$dark-edit: darken($secondary-color, 10%);
$dark-subscribe: darken($primary-color, 10%);
Copy link

Choose a reason for hiding this comment

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

This should be darken($subscribe, 10%); (in case $subscribe is set to something other than $primary-color).

Copy link
Author

Choose a reason for hiding this comment

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

Fixed

Comment on lines 38 to 39
$dark-subscribe: darken($primary-color, 10%);
$dark-favourite: darken($primary-color, 10%);
Copy link

Choose a reason for hiding this comment

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

This should be darken($favourite, 10%); (in case $favourite is set to something other than $primary-color).

Copy link
Author

Choose a reason for hiding this comment

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

Fixed

Comment on lines 22 to 26
{% if action == 'unflag' %}
{% set action_class = 'unsubscribe' %}
{% else %}
{% set action_class = 'subscribe' %}
{% endif %}
Copy link

Choose a reason for hiding this comment

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

Should be written as {% set action_class = action == 'unflag' ? 'unsubscribe' : 'subscribe' %}.

Copy link
Author

Choose a reason for hiding this comment

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

Changed

{% set title = "About document"|t %}
{{ _self.title(title, "document", node, content) }}
{% endif %}
{{ macros.title("About document", "document", node, content) }}
Copy link

Choose a reason for hiding this comment

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

"About document" must be translated, i.e. {{ macros.title("About document"|t, … }}.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed

Comment on lines 39 to 42
<div class="user">
{% include '@os2loop_theme/field/user-image.html.twig' with {'uid': user.account.id} %}
{% include '@os2loop_theme/field/username.html.twig' with {'uid': user.account.id, 'account': logged_in_user} %}
{% include '@os2loop_theme/field/user-name.html.twig' with {'uid': user.account.id} %}
</div>
Copy link

Choose a reason for hiding this comment

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

These 4 lines are duplicated in (at least) two template files and could be refactored into a shared template, user-info.html.twig, say. Furthermore, we should pass the entire user object to the template and let it get the parts it needs, e.g.

{% include '@os2loop_theme/user-info.html.twig' with {user: user} %}

Copy link
Author

Choose a reason for hiding this comment

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

A user-info.html.twig has been added.

<div class="col-md">
<div>
<a href="{{ url('entity.user.canonical', {'user': uid}) }}">{{ account.os2loop_user_given_name.value }} {{ account.os2loop_user_family_name.value }}</a>
<a href="{{ url('entity.user.canonical', {'user': uid}) }}">{{ drupal_field('os2loop_user_given_name', 'user', uid)['#items'].0.value }} {{ drupal_field('os2loop_user_family_name', 'user', uid)['#items'].0.value }}</a>
Copy link

Choose a reason for hiding this comment

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

Sometimes I hate Drupal! Do we really need ['#items'].0.value?
Can https://www.drupal.org/project/twig_field_value help clean this up?

Copy link
Author

Choose a reason for hiding this comment

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

Looks like it could yes.

Copy link
Author

Choose a reason for hiding this comment

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

After a rewrite with user-info.html.twig, it was made possible to grab the values from the user entity instead of calling drupal_field(), that turns:
drupal_field('os2loop_user_given_name', 'user', uid)['#items'].0.value
into
user['#user'].os2loop_user_given_name.value

@martinyde martinyde requested a review from rimi-itk May 5, 2021 19:22
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.

Just one final suggestion …

/**
* @file
* Default theme implementation for displaying a username.
* Template for displaying a username.
Copy link

Choose a reason for hiding this comment

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

Well, it actually displays a lot more. Maybe the comment should read “Template for displaying a user“ – or we could get rid of this template and the comment (cf. suggestion on inlining two templates).

Comment on lines 11 to 12
{% include '@os2loop_theme/field/user-image.html.twig' with {'uid': user['#user'].id} %}
{% include '@os2loop_theme/field/user-name.html.twig' with {'user': user} %}
Copy link

Choose a reason for hiding this comment

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

If these two (small) templates are not used anywhere else I think we should just put their content into this template. If inlining is not possible, we should pass the full user object to user-image.html.twig for consistency.

Copy link
Author

Choose a reason for hiding this comment

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

Combined into one

@martinyde martinyde requested a review from rimi-itk May 6, 2021 07:27
@martinyde martinyde merged commit f91f37e into develop May 6, 2021
@martinyde martinyde deleted the feature/subscriptions-modify branch May 6, 2021 08:55
martinyde added a commit that referenced this pull request Dec 15, 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.

3 participants