Skip to content

Fixes Jetpack Comments Validation Errors#2683

Closed
shahthepro wants to merge 3 commits intoAutomattic:masterfrom
shahthepro:master
Closed

Fixes Jetpack Comments Validation Errors#2683
shahthepro wants to merge 3 commits intoAutomattic:masterfrom
shahthepro:master

Conversation

@shahthepro
Copy link
Copy Markdown

@jeherve
Copy link
Copy Markdown
Member

jeherve commented Sep 8, 2015

It seems like your PR removes some of the changes introduced in 0d29082.

Could you make sure your PR only includes the changes you want to see in Jetpack?

Thanks!

@jeherve jeherve added Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Feature] Comments [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Sep 8, 2015
@jeherve jeherve added this to the 3.8 milestone Sep 8, 2015
@jeherve
Copy link
Copy Markdown
Member

jeherve commented Sep 8, 2015

Related: #1071

@shahthepro
Copy link
Copy Markdown
Author

@jeherve Check this commit shahthepro@4cbb498

It is what you meant, right?

@jeherve
Copy link
Copy Markdown
Member

jeherve commented Sep 8, 2015

Yes, exactly! 👍

@jeherve jeherve added [Status] Needs Review This PR is ready for review. and removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Sep 8, 2015
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm worried that there could be more than 1 comment form on a page, as specially if we are in a loop. In that case getElementById may not be reliable. Is there a CSS property that we can make use of?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@roccotripaldi Check this commit 27926fe

Haven't tested it though. But, It should work however.

@samhotchkiss samhotchkiss modified the milestones: 3.9, 3.8 Oct 22, 2015
Updated the selectors as per the suggestion from @roccotripaldi
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@shahthepro is the overflow: hidden necessary? it wasn't there previously and while testing this, I removed it and didn't notice any issue.

@eliorivero eliorivero added the [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. label Dec 30, 2015
@jeherve jeherve modified the milestones: 4.0, 3.9 Jan 12, 2016
@jeherve jeherve removed the [Status] Needs Review This PR is ready for review. label Jun 15, 2016
@jeherve jeherve modified the milestones: 4.2, 4.1 Jun 17, 2016
@jeherve jeherve modified the milestones: 4.3, 4.2 Jul 6, 2016
@richardmuscat richardmuscat modified the milestone: 4.3 Jul 7, 2016
@jeherve jeherve added this to the 4.4 milestone Jul 8, 2016
@jeherve jeherve added this to the 4.4 milestone Jul 8, 2016
@samhotchkiss samhotchkiss modified the milestones: Not Currently Planned, 4.4 Nov 9, 2016
@samhotchkiss
Copy link
Copy Markdown
Contributor

Moved out of the pipeline until @shahthepro addresses @eliorivero's concerns and rebases

@samhotchkiss
Copy link
Copy Markdown
Contributor

@eliorivero -- if you want to go ahead and make the changes you recommended here, that'd be great!

@eliorivero
Copy link
Copy Markdown
Contributor

Closing in favor of #6316 where the commits from author have been cherry picked.

@eliorivero eliorivero closed this Feb 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Feature] Comments [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants