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

LPS-113692 New Layout JSP Tags #88881

Closed
wants to merge 15 commits into from
Closed

LPS-113692 New Layout JSP Tags #88881

wants to merge 15 commits into from

Conversation

wincent
Copy link

@wincent wincent commented May 19, 2020

Manual forward from: https://github.com/wincent/liferay-portal/pull/270

One "unique" failure looks spurious/transitory.

Original message follows.


Fairly dense PR related to Layout JSP Tags. Kind of a follow up of #88326 (which in itself was https://github.com/wincent/liferay-portal/pull/245)

This PR pushes forward the tags adding some of the feedback gathered in Clay's own feat(@clayui/layout): create low-level layout utilities.

So, without further ado... the PR:

  • Creates an internal BaseContainerTag from which to extend further Layout components. This could be named BaseLayoutTag. Since it's internal, we can change it later if that makes more sense.
    • Controls the basic markup generation
    • Allows overriding processClassName so different tags can define which CSS classes are passed
    • Makes sure className is always supported and properly concatenated for all layout tags
    • Implements containerElement so it's possible to output something like td class="autofit-row" instead of always forcing a div
    • Adds support for DynamicAttributes so tags accept any HTML attribute even if not defined in the tld: <clay:container foo="fasd" aria-rol="button" aria-title="bar">...</clay:container>
  • Makes clay:container more generic and creates a specific clay:container-fluid tag which is the one we specifically use in DXP
  • Replaces all usages of clay:container with the new clay:container-fluid
  • Creates clay:content-col, clay:content-row and clay:content-section for the autofluid use case
  • Creates clay:sheet, clay:sheet-footer, clay:sheet-header and clay:sheet-section for the section case
  • Replaces raw markup for layout tags in blogs/blogs-web to see examples of clay:content* tags
  • Replaces raw markup for layout tags in depot/depot-web to see examples of clay:sheet* tags

@adolfopa, I think this is a fairly low risk PR, but would you mind having someone on your team take a look?

/cc @marcoscv-work, @bryceosterhaus, @eduardoallegrini

jbalsas added 15 commits May 19, 2020 09:01
…luid-container for better semantics and defaults
Starting at LPS-91526, the tag class is added to the request for performance and
backwards compatibility reasons, so we don't need to set the independent
attributes in the request and can use BeanPropertiesUtils to inspect the
necessary values in a safer way.
…y tags

This is done to simplify adding custom attributes to the layout containers
such as aria or any supported html attribute. Attributes are set without
any further processing.

This adds out of the box support for the `id` attribute. This is the reason
to remove it from the tld. Since we're not processing it in any way, it's
best to keep it inline with other html attributes. Being supported through
the new dynamic attributes support also means that this should not necessarily
introduce a breaking change.
@liferay-continuous-integration
Copy link
Collaborator

Closing pull request because all liferay-portal pullrequests sent to Brian Chan must be sent by using ci:forward on a pull request that was sent to someone else.

@wincent
Copy link
Author

wincent commented May 19, 2020

ci:reopen

@wincent wincent changed the title Lps 113692 LPS-113692 New Layout JSP Tags May 19, 2020
@brianchandotcom
Copy link
Owner

Merged. Thank you.
View total diff: 24ebc25...d0b6c95

@brianchandotcom
Copy link
Owner

@jbalsas @wincent this looks great. Good job guys. Even in the Java code!

@jbalsas jbalsas deleted the LPS-113692 branch October 30, 2020 17: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.

4 participants