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

fieldmanager ui update #468

Merged
merged 82 commits into from Feb 26, 2021
Merged

fieldmanager ui update #468

merged 82 commits into from Feb 26, 2021

Conversation

stevenslack
Copy link
Contributor

@stevenslack stevenslack commented Feb 20, 2016

  • greater spacing between fields
  • repeatable / sortable UI improvements
  • media upload field provides more info about file uploaded
  • tab UI updates
  • new field CSS class which wraps just the label, field and description
  • labels and descriptions style updates

@mboynes
Copy link
Contributor

mboynes commented Feb 20, 2016

  • Test descriptions in horizontal tab groups
  • Test descriptions in vertical tab groups
  • Test descriptions in nested groups (2 and 3 levels deep)

@mboynes mboynes modified the milestone: 1.1.0 Feb 22, 2016
* div to paragraph tag to prevent hiding from JS which targets sibling divs
* special handling for displaying the before description before the UL element in tabbed groups
Copy link
Contributor

@montchr montchr left a comment

Choose a reason for hiding this comment

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

Looks good to me overall! I have one blocking question about the use of ID selectors in one place, and some suggestions about using more prominent section divider comments in CSS.

css/fieldmanager-group-tabs.css Outdated Show resolved Hide resolved
css/fieldmanager.css Show resolved Hide resolved
css/fieldmanager.css Show resolved Hide resolved
css/fieldmanager.css Show resolved Hide resolved
Copy link
Contributor

@montchr montchr left a comment

Choose a reason for hiding this comment

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

🌀

} else {
var preview = 'Uploaded file: ';
preview += wp.media.string.link( props );
preview += '<a href="#"><span class="dashicons dashicons-media-document"></span></a>';
Copy link
Member

Choose a reason for hiding this comment

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

Is there a risk of these media icon classes becoming out of sync with the filtered icon class used in PHP?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dlh01 I think the filter may not have a solid use case. I pushed up a change that will set the icon depending on the attachment icon type. See https://github.com/alleyinteractive/wordpress-fieldmanager/pull/468/files#diff-7f0fb55a63140bb4d44360770c1ec06ff18be39f6c9e359f8f7370bca480efcbR81-R83

Let me know what you think about his change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are now relying on the media type from the attachment to output the correct icon.

php/class-fieldmanager-field.php Outdated Show resolved Hide resolved
php/class-fieldmanager-media.php Outdated Show resolved Hide resolved
php/class-fieldmanager-media.php Outdated Show resolved Hide resolved
@dlh01
Copy link
Member

dlh01 commented Jan 19, 2021

Couple potential edge cases I noticed while testing this branch on a client site:

Checkbox with a long label in the sidebar of the block editor breaks to a new line:

Screen Shot 2021-01-18 at 11 45 43 PM

Checkboxes within a tabbed group start to the left of the tab (not actually sure whether this is a defect):

Screen Shot 2021-01-18 at 11 45 33 PM

These cases aside, plus the few notes inline, the branch is looking really good to me. I'm excited to get it merged! Let me know what you think about the comments and how it'd be best to handle them, including in a follow-up PR.

@stevenslack
Copy link
Contributor Author

@dlh01 I addressed your feedback for the checkboxes wrapping to the next line in 3b6e936. For the checkboxes in the tabs, this isn't really a defect as much as it is just the default behavior.

@dlh01
Copy link
Member

dlh01 commented Feb 26, 2021

@stevenslack Re. 9dd2cc2, where does the class .fm-checkbox come from? On the client site where that screenshot originated, the class isn't present, so the fix has no effect.

@stevenslack
Copy link
Contributor Author

@dlh01 The wrapper element, fm-item does not get updated with a class representing the type of field which it encapsulates. The commit 9dd2cc2 is incorrect in this case.

The issue you are referring to where a checkbox with a long label in the sidebar of the block editor is present on master as well and is not caused by this feature branch. This issue should be addressed in a separate UI update for FM.

@dlh01
Copy link
Member

dlh01 commented Feb 26, 2021

Interestingly, I see that an instance of .fm-checkbox was added to the codebase in 2013: cb4fe88. Perhaps there used to be such a class-based class added.

As for the checkbox label, it looks as expected to me in master:

Screen Shot 2021-02-26 at 4 30 17 PM

Regardless, if you'd prefer to move the discussion to a separate issue, let me know.

@stevenslack
Copy link
Contributor Author

@dlh01 given the age on this PR I'd like to move the discussion of the checkbox label wrapping to another issue.

dlh01
dlh01 approved these changes Feb 26, 2021
Copy link
Member

@dlh01 dlh01 left a comment

Choose a reason for hiding this comment

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

🌳 Let's go!

@dlh01 dlh01 merged commit fa94a16 into master Feb 26, 2021
@dlh01 dlh01 deleted the fm-ui-update branch February 26, 2021 22:37
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.

None yet

6 participants