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

WIP: Social links block. #14855

Closed
wants to merge 17 commits into from
Closed

WIP: Social links block. #14855

wants to merge 17 commits into from

Conversation

nicolad
Copy link
Member

@nicolad nicolad commented Apr 6, 2019

Description

Implements: #1873

References

slack discussion
Prototype

How has this been tested?

Screenshots

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@gziolo gziolo added [Type] Task Issues or PRs that have been broken down into an individual action to take [Feature] Blocks Overall functionality of blocks labels Apr 9, 2019
@Soean
Copy link
Member

Soean commented Apr 9, 2019

I haven't tested it in the editor yet, but we should follow #14770 and add a block.json file for new blocks.

@Soean Soean added the New Block Suggestion for a new block label Apr 9, 2019
@mapk
Copy link
Contributor

mapk commented Apr 10, 2019

@nicolad thanks for taking this on!

I tried testing this PR and couldn't get through it. Here's a screencast of my experience. I'm also aware this is probably a work in progress so feel free to ignore my comments right now.

social

A couple design notes:

  1. The block icon should not be a social media site. Maybe try a "sharing" icon like: https://material.io/tools/icons/?icon=share&style=baseline

  2. The + button next to the URL is a bit confusing for me. Does it submit the URL I entered? or does it add a new line? I couldn't get it to work.

@nicolad
Copy link
Member Author

nicolad commented Apr 12, 2019

I haven't tested it in the editor yet, but we should follow #14770 and add a block.json file for new blocks.

Thank you for review, had added block.json.

@nicolad
Copy link
Member Author

nicolad commented Apr 12, 2019

@nicolad thanks for taking this on!

I tried testing this PR and couldn't get through it. Here's a screencast of my experience. I'm also aware this is probably a work in progress so feel free to ignore my comments right now.

social

A couple design notes:

  1. The block icon should not be a social media site. Maybe try a "sharing" icon like: https://material.io/tools/icons/?icon=share&style=baseline
  2. The + button next to the URL is a bit confusing for me. Does it submit the URL I entered? or does it add a new line? I couldn't get it to work.

Yes, it is in progress. Was a little busy and did not had the chance to make a lot of things here. Will have more time upcoming days and will try to do more.
Anyway, thanks for the feedback it's always great to have an additional pair of eyes.

@Soean
Copy link
Member

Soean commented Apr 12, 2019

Thank you for review, had added block.json.

Looks good, save should also live in its own file, see #14882 for example.


icon,

category: 'layout',
Copy link
Member Author

Choose a reason for hiding this comment

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

What will be the better category fit?

Copy link
Member

Choose a reason for hiding this comment

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

widget?

@nicolad
Copy link
Member Author

nicolad commented Apr 12, 2019

Thanks @Soean, updated.
I started doing this block by copying button block.
Could you please suggest other suitable blocks at which I can take a look at the implementation?

@CalumChilds
Copy link

I think the icon shouldn't be the Facebook logo, as people would think it is a Facebook-specific block. Instead, I recommend replacing it with a Share icon.

return {
addLink() {
const created = createBlock( 'core/social-link', { verticalAlignment: 'top' } );
dispatch( 'core/editor' ).insertBlock( created, undefined, clientId );
Copy link
Member Author

Choose a reason for hiding this comment

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

Hello @gziolo, on this action I want to create a sub-child, for some reason it won't fire the needed result.
I am not sure what I am doing wrong here.

Copy link
Member Author

Choose a reason for hiding this comment

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

It seems that insertBlock isn't the best approach here, tried a different solution, based of columns block approach: 792401a

@gziolo gziolo requested a review from a team May 21, 2019 14:29
@karmatosed
Copy link
Member

As this had needs design feedback, I thought I would give the current PR a go. I am not sure if something went wrong as the experience I don't think was what intended. For example on loading I saw this:

image

Then I get:

image

I'd be happy to test again but I do think either something is a little weird my end or the PR needs a little nurturing. If this is a 'me' issue, I would be happy to review based on screenshots of the flow.

@nicolad
Copy link
Member Author

nicolad commented Jul 6, 2019

hey @karmatosed, you're right - this PR needs a little bit of nurture since it's still a work in progress. I will let you know when it's ready to be reviewed. Thanks.

@nicolad nicolad self-assigned this Jul 10, 2019
@noisysocks noisysocks added the [Status] In Progress Tracking issues with work in progress label Jul 10, 2019
@mkaz
Copy link
Member

mkaz commented Aug 1, 2019

@nicolad here is a quick GIF demo of what I have currently in place. I put that together prior to seeing the prototype from #1873

I have it as a separate plugin which is pretty rough with the interactions. I can put it up somewhere if we want iterate, but after looking at the prototype in that issue and your work, maybe I can combine the two efforts to some degree

social-icons-demo

@mkaz
Copy link
Member

mkaz commented Aug 2, 2019

@nicolad if it is alright with you, I'm going to combine our efforts, though I'm not quite sure how.
I don't think I can push to your branch, and not sure if you can push to one I create here.

@nicolad
Copy link
Member Author

nicolad commented Aug 3, 2019

Hello @mkaz I am closing this PR in favor of new-one based on master branch: #16897

@nicolad nicolad closed this Aug 3, 2019
@nicolad nicolad removed their assignment Sep 29, 2019
@nicolad nicolad added [Status] Not Implemented Issue/PR we will (likely) not implement. and removed [Status] In Progress Tracking issues with work in progress labels Aug 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Blocks Overall functionality of blocks Needs Design Feedback Needs general design feedback. New Block Suggestion for a new block [Status] Not Implemented Issue/PR we will (likely) not implement. [Type] Task Issues or PRs that have been broken down into an individual action to take
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants