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

feat(file): Added download attributes. - OEL-2772 #573

Merged
merged 7 commits into from
Aug 1, 2024

Conversation

tibi2303
Copy link
Collaborator

No description provided.

Copy link

github-actions bot commented Jan 12, 2024

🚀 Deployed on https://preview-573--oelibrary.netlify.app

@github-actions github-actions bot temporarily deployed to pull request January 12, 2024 14:23 Inactive
@github-actions github-actions bot temporarily deployed to pull request March 6, 2024 10:42 Inactive
@julien-
Copy link

julien- commented Mar 25, 2024

@github-actions github-actions bot temporarily deployed to pull request July 26, 2024 09:30 Inactive
@github-actions github-actions bot temporarily deployed to pull request July 26, 2024 15:17 Inactive
Comment on lines 141 to 158
{% if _download is not empty and _download.icon is not empty %}
{% set _download = _download|merge({
icon: _download.icon
}) %}
{% else %}
{% set _download = _download|merge({
icon: {
name: "download",
size: "fluid",
path: _icon_path,
},
}) %}
{% endif %}
{% if _download.attributes is empty %}
{% set _download = _download|merge({
attributes: create_attribute()
}) %}
{% endif %}

Choose a reason for hiding this comment

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

Suggested change
{% if _download is not empty and _download.icon is not empty %}
{% set _download = _download|merge({
icon: _download.icon
}) %}
{% else %}
{% set _download = _download|merge({
icon: {
name: "download",
size: "fluid",
path: _icon_path,
},
}) %}
{% endif %}
{% if _download.attributes is empty %}
{% set _download = _download|merge({
attributes: create_attribute()
}) %}
{% endif %}
{% set _download = _download|merge({
icon: _download.icon|default({}|merge({
name: "download",
size: "fluid",
path: _icon_path,
}),
attributes: _download.attributes|default(create_attribute())|merge({
download: true,
target: _blank,
})
}) %}

Choose a reason for hiding this comment

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

Maybe I am missing something but what we are trying to do is nested array merges. With the above, we set into icon the value that it already has, defaulted to an empty object in case it's not there, and then merged with the default icon settings.
For attributes we do the same: if it was passed down in _download.attributes we use it, otherwise we default to a clean one. Then we merge, not set, the values.

Comment on lines 161 to 164
attributes: _download.attributes
.setAttribute('download', true)
.setAttribute('target', '_blank')
.addClass(['standalone', 'align-self-center', 'd-inline-block', 'mt-1', 'mt-md-0', 'flex-shrink-0'])

Choose a reason for hiding this comment

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

Suggested change
attributes: _download.attributes
.setAttribute('download', true)
.setAttribute('target', '_blank')
.addClass(['standalone', 'align-self-center', 'd-inline-block', 'mt-1', 'mt-md-0', 'flex-shrink-0'])
attributes: _download.attributes
.addClass(['standalone', 'align-self-center', 'd-inline-block', 'mt-1', 'mt-md-0', 'flex-shrink-0'])

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It was failing like this, it's more or less how you suggested now but without the attributes merge.

Copy link

Choose a reason for hiding this comment

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

Hello misters,

Why not keeping my original logic #583 ?

  1. Set the BCL default attributes
  2. Merge with possible provided child that can eventually remove/update/add attributes

For example, in your case you are forcing the download=true, I do not need it sometimes
See : https://historia.europa.eu/en/about-us/press (icon changed + attributes changed)

path: "/example.html",
icon: {
name: "cloud-download",
size: "fluid",

Choose a reason for hiding this comment

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

I wish we could test this with a different icon set :) But it's ok.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In the test you can see that it's not populated... I could add but we have only that sprite :)) and i think it's enough that it's not populated if you dont add it (that means you can change it how you want).

[skip chromatic]
@github-actions github-actions bot temporarily deployed to pull request July 31, 2024 14:15 Inactive
@github-actions github-actions bot temporarily deployed to pull request August 1, 2024 12:45 Inactive
@tibi2303 tibi2303 merged commit 6f6e668 into development Aug 1, 2024
8 of 9 checks passed
@tibi2303 tibi2303 deleted the OEL-2772-#default# branch August 1, 2024 13:53
tibi2303 added a commit that referenced this pull request Aug 1, 2024
@tibi2303 tibi2303 restored the OEL-2772-#default# branch August 1, 2024 13:54
tibi2303 added a commit that referenced this pull request Aug 1, 2024
@github-actions github-actions bot temporarily deployed to pull request August 1, 2024 14:01 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants