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

Use default icons for FieldPanels and add docs/tests for Panel icon customisation #10320

Merged
merged 17 commits into from
Apr 18, 2023

Conversation

laymonage
Copy link
Member

@laymonage laymonage commented Apr 6, 2023

Fixes #10274

  • Add documentation on how to set custom icons to panels, to a similar extent to what we do for StreamField
  • Review possible default icons based on what we do for StreamField
  • Add unit tests for the icon support to make sure default and custom icons are correctly picked up

Notes:

  • I changed the default icon mechanism to check the model field first (instead of the form field). This is because model fields do not always have a 1:1 mapping to the form fields. For example, models.TextField and models.CharField both use forms.CharField.

    I spoke with Ben and he said we should use the pilcrow icon for TextFields and RichTextFields, but not for CharField. This is because the pilcrow generally represents a paragraph, which doesn't mean much in the context of CharFields. If we check for the form field, we cannot differentiate between TextField and CharField, unless we check the widget as well. Thus, I made the code to check for the model field first.

  • That said, apparently FieldPanel can be used without a model field:

    def test_non_model_field(self):
    # defining a FieldPanel for a field which isn't part of a model is OK,
    # because it might be defined on the form instead
    field_panel = FieldPanel("barbecue").bind_to_model(Page)
    # however, accessing db_field will fail
    with self.assertRaises(FieldDoesNotExist):
    field_panel.db_field

    So, I added a fallback code to use the form field to check for the default icon if the field does not exist on the model.

  • For the same reason as the model vs form fields discrepancy, this means that the RegexField default will not be used unless either (a) the user creates a custom RegexField model field, or (b) the specified field does not exist on the model, and the form field is RegexField.

  • I tweaked the field class checking to also look through the MRO. This allows us to use TaggableManager to define the default icon, and it works for both TaggableManager and ClusterableTaggableManager. This also allows custom fields that are subclasses of the known fields to use our default icons.

  • For chooser panels, this was slightly trickier. We need to check if the field is a ForeignKey, and if so, we have two options that I can think of:

    • Look at the remote field's model. If it's a subclass of Page, Image, or Document, use the custom icon accordingly.
    • Look at our form field overrides. Then, see the widget's icon based on the override.
  • I've revamped the docs for the panel types and the Panel API. There are some liberties that I took, and we might want Matthew to review the docs changes.

To test this, I've made a branch based on @thibaudcolas' "everything page" branch:
https://github.com/laymonage/bakerydemo/tree/feature/10274-everything-page-icons

The branch has been updated to remove the custom icon="..." argument in the FieldPanels. You can verify that even with the argument removed, the default icon will still be shown for the known fields.

Please check the following:

  • Do the tests still pass?1
  • Does the code comply with the style guide?
    • Run make lint from the Wagtail root.
  • For Python changes: Have you added tests to cover the new/fixed behaviour?
  • For front-end changes: Did you test on all of Wagtail’s supported environments?2
    • Please list the exact browser and operating system versions you tested:
    • Please list which assistive technologies 3 you tested:
  • For new features: Has the documentation been updated accordingly?

Please describe additional details for testing this change.

Footnotes

  1. Development Testing

  2. Browser and device support

  3. Accessibility Target

@squash-labs
Copy link

squash-labs bot commented Apr 6, 2023

Manage this branch in Squash

Test this branch here: https://laymonagepanel-icons-mx0in.squash.io

self.bound_field.field refers to the form field instance.

Some model fields share the same form field type by default, e.g. models.TextField uses forms.CharField
This removes the parameters that come from the base Panel class (e.g.
heading, classname, etc.) from the specific panel types to avoid
repetition. Instead, these parameters are properly documented in the
separate "Panel customisation" section. They are also explicitly listed
in the Panel API reference.
URLField/URLBlock: link-external
RichTextField/RichTextBlock: pilcrow
FloatField/DecimalField/FloatBlock/DecimalBlock: decimal
RegexField/RegexBlock: regex
PageChooserBlock: doc-empty-inverse
IntegerField: None (will use the arrow-down-big)
IntegerBlock: placeholder
Copy link
Member

@thibaudcolas thibaudcolas left a comment

Choose a reason for hiding this comment

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

Nice one @laymonage! All working as far as I can tell, just needed to SVGO-compress the new icons and add licensing information within.


This panel allows for the creation of a "cluster" of related objects over a join to a separate model, such as a list of related links or slides to an image carousel. For a full explanation on the usage of ``InlinePanel``, see :ref:`inline_models`.
This panel allows for the creation of a "cluster" of related objects over a join to a separate model, such as a list of related links or slides to an image carousel. For a full explanation on the usage of ``InlinePanel``, see :ref:`inline_models`. To save space, you can :ref:`collapse the panel by default <collapsible>`.
Copy link
Member

Choose a reason for hiding this comment

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

Nice one!

@thibaudcolas thibaudcolas added this to the 5.0 milestone Apr 18, 2023
@thibaudcolas thibaudcolas merged commit 68cea04 into wagtail:main Apr 18, 2023
4 of 12 checks passed
thibaudcolas added a commit to thibaudcolas/wagtail that referenced this pull request Apr 18, 2023
@laymonage laymonage mentioned this pull request Apr 18, 2023
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Panel icons: documentation, review default icons, unit tests
3 participants