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

Byline: adding documentation, moving to main title #2452

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

Conversation

Garneauma
Copy link
Contributor

@Garneauma Garneauma commented Dec 11, 2024

  • Adding byline documentation so that it can be deprecated
  • Adding "Main page title with byline" variation to the Main page title (this variation is limited to news pages on Canada.ca)

Related to WET-494

@Garneauma
Copy link
Contributor Author

@duboisp I have set the status of the byline component to deprecated. However, if we want to get this pull request out without triggering a major release, I can change it to demoted instead.

@Garneauma
Copy link
Contributor Author

Pre-approved upon successful review.

Copy link
Member

@duboisp duboisp left a comment

Choose a reason for hiding this comment

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

See my comment inline.

Note: We will need to discuss if we apply that by default to the last paragraph in the hgroup or if we require this to be activated by the author by using a CSS class. The second option will allow us to use the paragraph under the h1 for something else considering that the trial currently is to remove the byline.

},
"modified": "2024-12-10",
"componentName": "byline",
"status": "deprecated",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"status": "deprecated",
"status": "demoted",

We will formally deprecate it at our next major version

"en": "Institutional byline - default",
"fr": "Institution responsable - par défaut"
},
"status": "deprecated",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"status": "deprecated",
"status": "demoted",

Comment on lines +94 to +100
"en": [
{
"@type": "source-code",
"description": "Code sample:",
"code": "<p class=\"gc-byline\"><strong>From <a href=\"#\">[Institution name]</a></strong></p>"
}
],
Copy link
Member

Choose a reason for hiding this comment

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

Add each possible variation as code sample

[h1] + <p class="gc-byline">From <a href=\"#\">[Institution name]</a></p>
[h1] + <p><strong>From <a href=\"#\">[Institution name]</a></strong></p>

(You already have that one)

[h1] + <p class="gc-byline"><strong>From <a href=\"#\">[Institution name]</a></strong></p>

{
"@id": "_:cs_byline_2",
"name": "Institutional byline - version 2",
"status": "deprecated",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"status": "deprecated",
"status": "demoted",

{
"@id": "_:cs_byline",
"name": "Institutional byline",
"status": "deprecated",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"status": "deprecated",
"status": "demoted",

Comment on lines +7 to +9
{%- if page.byline -%}
<p>{% if page.language == "fr" %}De&nbsp;:{% else %}From:{% endif %} <a href="{{ page.byline.url }}">{{ page.byline.institution }}</a></p>
{%- endif -%}
Copy link
Member

Choose a reason for hiding this comment

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

Do we make it default or should we make the author to enable this, like by using the gc-byline class on the last paragraph?

We just need to think long term, do we want to lock down the last paragraph for a byline that is now half demote and still alive on news pages knowing that there is an intention to get it replaced by gc-contributor instead. Let have an internal discussion about it.

cc @Ricokola

I do think we should have something like to allow us to re-use that last paragraph in the "hgroup" heading section for other purpose:

Suggested change
{%- if page.byline -%}
<p>{% if page.language == "fr" %}De&nbsp;:{% else %}From:{% endif %} <a href="{{ page.byline.url }}">{{ page.byline.institution }}</a></p>
{%- endif -%}
{%- if page.byline -%}
<p class="gc-byline">{% if page.language == "fr" %}De&nbsp;:{% else %}From:{% endif %} <a href="{{ page.byline.url }}">{{ page.byline.institution }}</a></p>
{%- endif -%}

or

Suggested change
{%- if page.byline -%}
<p>{% if page.language == "fr" %}De&nbsp;:{% else %}From:{% endif %} <a href="{{ page.byline.url }}">{{ page.byline.institution }}</a></p>
{%- endif -%}
<hgroup class="with-byline">
....
{%- if page.byline -%}
<p>{% if page.language == "fr" %}De&nbsp;:{% else %}From:{% endif %} <a href="{{ page.byline.url }}">{{ page.byline.institution }}</a></p>
{%- endif -%}
</hgroup>

@@ -32,4 +32,9 @@ hgroup#wb-cont {
h1 {
margin-top: 0;
}

p:last-child {
Copy link
Member

Choose a reason for hiding this comment

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

See the inner comment in the main-page-title include file

Suggested change
p:last-child {
&with-byline p:last-child {
Suggested change
p:last-child {
p:last-child.gc-byline {

@@ -91,6 +91,16 @@
"title": "Titre principal superposé aligné à droite",
"language": "fr",
"path": "main-page-title-stacked-align-right-fr.html"
},
{
"title": "Main page title with byline",
Copy link
Member

Choose a reason for hiding this comment

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

Let's mention this is context specific to news article

Suggested change
"title": "Main page title with byline",
"title": "Main page title with byline for news articles",

"path": "main-page-title-byline-en.html"
},
{
"title": "Titre principal avec institution responsable",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"title": "Titre principal avec institution responsable",
"title": "Titre principal avec institution responsable pour les article de nouvelles",

Comment on lines +203 to +204
"en": "Main page title with byline",
"fr": "Titre principal avec institution responsable"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"en": "Main page title with byline",
"fr": "Titre principal avec institution responsable"
"en": "Main page title with byline for news articles",
"fr": "Titre principal avec institution responsable pour les article de nouvelles"

@duboisp duboisp assigned Garneauma and unassigned duboisp Jan 15, 2025
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