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

x-live-blog-post component #474

Merged
merged 36 commits into from
Jun 23, 2020
Merged

x-live-blog-post component #474

merged 36 commits into from
Jun 23, 2020

Conversation

tbergmen
Copy link
Contributor

@tbergmen tbergmen commented Jun 17, 2020

x-live-blog-post renders live blog posts with an updated design.

Before:

Screenshot 2020-06-17 10 26 53

After:

Screenshot 2020-06-17 10 25 29

Related issue:
#446

@tbergmen tbergmen added component A new component in development x-content-block Pull requests for development of the x-content-block component labels Jun 17, 2020
@tbergmen tbergmen requested a review from a team as a code owner June 17, 2020 09:56
@apaleslimghost apaleslimghost temporarily deployed to x-dash-x-content-block-k96nbgw June 17, 2020 09:56 Inactive
@tbergmen tbergmen requested a review from a team June 17, 2020 09:59
@tbergmen tbergmen temporarily deployed to x-dash-x-content-block-k96nbgw June 17, 2020 10:08 Inactive
@tbergmen
Copy link
Contributor Author

I have rebased this branch on master.

Copy link
Contributor

@andygout andygout left a comment

Choose a reason for hiding this comment

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

Name:

I wonder if based on the PR description (below) whether its name should be changed from x-content-block to x-blog-post?

x-content-block renders live blog posts with an updated design.
I expect FT.com and the App feature other page sections that could be described as a 'content block' but which could not use this component, so I wonder if renaming so as to finely define its scope.

Tests:

There do not seem to be any. Even if the component is currently only receiving props and spitting them out, it is still worth having the tests in place for the eventuality that it later becomes more complex. The x-privacy-manager component was pretty thorough in this regard: https://github.com/Financial-Times/x-dash/pull/458/files.

Accessibility:

Has the component been tested for accessibility?

components/x-content-block/package.json Outdated Show resolved Hide resolved
components/x-content-block/readme.md Outdated Show resolved Hide resolved
components/x-content-block/readme.md Outdated Show resolved Hide resolved
components/x-content-block/src/ContentBlock.scss Outdated Show resolved Hide resolved
components/x-content-block/src/ContentBlock.scss Outdated Show resolved Hide resolved
import { h } from '@financial-times/x-engine';
import styles from './ContentBlock.scss';

// this function extracts time in HH:mm format from a given date
Copy link
Contributor

Choose a reason for hiding this comment

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

Does o-date or any other existing package do this? Seems like it must be a common use case for elsewhere on FT.com.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now using o-date for rendering the exact date.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can the code that calculates the time ll.5-23 not use one of the o-date methods listed under the JavaScript header?

It seems like what you are trying to achieve must be a common use case and I would be surprised if it was not achievable with one of these methods.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I avoid using Origami Javascript in the x-dash component because of this comment here: #446 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, interesting.

@tbergmen tbergmen temporarily deployed to x-dash-x-content-block-k96nbgw June 18, 2020 14:46 Inactive
@tbergmen tbergmen temporarily deployed to x-dash-x-content-block-k96nbgw June 19, 2020 08:12 Inactive
@tbergmen tbergmen temporarily deployed to x-dash-x-content-block-k96nbgw June 19, 2020 10:37 Inactive
@tbergmen
Copy link
Contributor Author

@andygout For accessibility, I added more descriptive aria labels to the sharing buttons. Apart from that, I couldn't see any issues. Do you have anything else in mind?

@tbergmen tbergmen temporarily deployed to x-dash-x-content-block-k96nbgw June 19, 2020 10:55 Inactive
@tbergmen tbergmen temporarily deployed to x-dash-x-content-block-k96nbgw June 19, 2020 10:56 Inactive
@tbergmen tbergmen temporarily deployed to x-dash-x-content-block-k96nbgw June 19, 2020 11:06 Inactive
@tbergmen tbergmen requested a review from andygout June 19, 2020 11:07
@andygout
Copy link
Contributor

Re. accessibility, I think as long as you have tested it yourself with a screenreader and found that the component can be navigated in an understandable manner (maybe in the context of an app) then that should be okay.

andygout
andygout previously approved these changes Jun 19, 2020
Copy link
Contributor

@andygout andygout left a comment

Choose a reason for hiding this comment

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

Providing you have checked the component for accessibility using a screen reader and maybe also keyboard-only, I would say this is good to go.

Please be sure to follow the Release Guidelines, which requires that new components have been well checked.

@tbergmen tbergmen requested a review from andygout June 22, 2020 09:04
@tbergmen tbergmen changed the title x-content-block component x-live-blog-post component Jun 22, 2020
andygout
andygout previously approved these changes Jun 22, 2020
Copy link
Contributor

@andygout andygout left a comment

Choose a reason for hiding this comment

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

Confirmed offline:

Yes, we have been testing it in next-article with the current live-blogs architecture. And, we will release it behind a flag initially.

@apaleslimghost
Copy link
Member

i see this has been tested in next-article, has it been tested in ft-app yet? that's a requirement for release

@tbergmen
Copy link
Contributor Author

@andygout @apaleslimghost

I tested the component in ft-app as well. The only issue was that the share buttons wouldn't work in the app. Therefore I created a PR that adds the option to hide share buttons. - #494

The rest of the features work well in the app.

@andygout
Copy link
Contributor

So #494 and this PR will be included in the same release?

@tbergmen
Copy link
Contributor Author

So #494 and this PR will be included in the same release?

@andygout Yes, I'm planning to merge #494 into this branch before merging this to master. Do you think that would work?

@tbergmen
Copy link
Contributor Author

@andygout I tested the component including the latest changes on next-article and on ft-app. It works well in both platforms.

Do you think we can go ahead with the release today?

@tbergmen tbergmen merged commit f564713 into master Jun 23, 2020
@tbergmen tbergmen deleted the x-content-block branch June 23, 2020 13:43
rowanbeentje pushed a commit that referenced this pull request Nov 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component A new component in development x-content-block Pull requests for development of the x-content-block component
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants