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

Publish fixes #1716

Merged
merged 6 commits into from
Jul 2, 2018
Merged

Publish fixes #1716

merged 6 commits into from
Jul 2, 2018

Conversation

neb-b
Copy link

@neb-b neb-b commented Jun 28, 2018

Fixes

Disabled the file input when publishing === true
Fix an issue with form error styling at the bottom of the publish form
#1714
#1647

Moved button styling to be specific to the transactions table because it was affecting buttons it shouldn't.

@tzarebczan
Copy link
Contributor

Tested and working as expected for me. When I edit the name, I get the warning about selecting a file. Publish works after going down that route. Also able to change the name back and edit as expected.

@tzarebczan tzarebczan requested a review from kauffj July 2, 2018 19:05
@lbry-bot lbry-bot assigned kauffj and unassigned neb-b Jul 2, 2018
Copy link
Member

@kauffj kauffj left a comment

Choose a reason for hiding this comment

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

I would like the application of the tourniquitting CSS to be improved and then you can merge this without another review.

{!bid && <div>{__('A bid amount is required')}</div>}
{!tosAccepted && <div>{__('You must agree to the terms of service')}</div>}
</div>
!isFormValid && (
Copy link
Member

Choose a reason for hiding this comment

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

This is fine, but it's worth considering in the future how at least some of these could be funneled directly to the fields rather than all shown in a global area.

Copy link
Author

Choose a reason for hiding this comment

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

They are shown in the field, this is just extra help at the bottom of the page so it isn't confusing why the publish button is greyed out.

Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Member

Choose a reason for hiding this comment

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

(On second thought, this still feels somewhat wrong. Maybe the superior pattern is one that provides a link to scroll to the fields with errors? But obviously non-blocking)

Copy link
Author

Choose a reason for hiding this comment

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

I like that idea 👍

@@ -99,6 +99,15 @@ table.table--transactions {
}
td:nth-of-type(3) {
width: 22.5%;

.btn--link {
Copy link
Member

Choose a reason for hiding this comment

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

I like this tourniquitting CSS but I dislike how it is being applied. Is tourniqueting text really just a property of button links when they're inside the 3rd td element of a <table.table--help>? This seems unlikely.

If we're struggling to find a clear global rule for these, I think it makes more sense to apply turniquitting as either a table or cell level class property.

Copy link
Member

Choose a reason for hiding this comment

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

Also, this breaks BEM

@lbry-bot lbry-bot assigned neb-b and unassigned kauffj Jul 2, 2018
@lbry-bot lbry-bot assigned neb-b and unassigned neb-b Jul 2, 2018
@lbry-bot lbry-bot assigned neb-b and unassigned neb-b Jul 2, 2018
@neb-b neb-b merged commit 9a8d487 into master Jul 2, 2018
@neb-b neb-b deleted the publish-fixes branch September 10, 2018 19:27
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.

3 participants