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

MWPW-137717 | New block - Basic Brick & Masonry #1515

Merged
merged 51 commits into from
Dec 7, 2023

Conversation

aishwaryamathuria
Copy link
Contributor

@aishwaryamathuria aishwaryamathuria commented Nov 9, 2023

This is a new Milo block that allows us to create a grid with bricks of size (full, half, thwo-third or one third) widths.
These are arranged into a masonry layout once section metadata is applied.
The bricks can be authored as fragments or directly on docx and will be arranged via section metadata.

  1. The bricks will be authored as individual blocks and then arranged via the masonry layout.
  2. The layout is not fixed, the different section layouts can be paired together (like 1:1:1 followed by 1:2 ).
  3. Background option for color + image (multiple viewport).
  4. Option to author multiple background image for different media ports.
  5. Text alignment - left, center, align-center.
  6. Default text sizes with override option.
  7. Clickable brick (Only if brick has single link)

Adding poc links -
https://masonry--milo--aishwaryamathuria.hlx.page/drafts/mathuria/masonry/poc/poc-1
https://masonry--milo--aishwaryamathuria.hlx.page/drafts/mathuria/masonry/poc/poc-2

Resolves: MWPW-137717

Test URLs:

Copy link
Contributor

aem-code-sync bot commented Nov 9, 2023

Hello, I'm the AEM Code Sync Bot and I will run some test suites that validate the page speed.
In case there are problems, just click the checkbox below to rerun the respective action.

  • Re-run PSI Checks

Copy link
Contributor

aem-code-sync bot commented Nov 9, 2023

Page Scores Audits Google
/drafts/mathuria/masonry/poc/poc-2?martech=off PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI

@aishwaryamathuria aishwaryamathuria added new-feature New block or other feature needs-verification PR requires E2E testing by a reviewer labels Nov 9, 2023
Copy link
Contributor

aem-code-sync bot commented Nov 9, 2023

Page Scores Audits Google
/drafts/mathuria/masonry/poc/poc-2?martech=off PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI

Copy link

codecov bot commented Nov 9, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (5ce8eef) 95.55% compared to head (fe3e823) 95.65%.
Report is 12 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1515      +/-   ##
==========================================
+ Coverage   95.55%   95.65%   +0.09%     
==========================================
  Files         150      150              
  Lines       38728    38783      +55     
==========================================
+ Hits        37008    37096      +88     
+ Misses       1720     1687      -33     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@aishwaryamathuria aishwaryamathuria added the run-nala Run Nala Test Automation against PR label Nov 9, 2023
Copy link
Member

@auniverseaway auniverseaway left a comment

Choose a reason for hiding this comment

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

I'll let others chime in more, but I think basic-brick should just be brick.

This would be like calling a product, "iPhone Basic" or "MacBook Basic"

I think the omission of "advanced" or "pro" implies this is basic... and all blocks should have basic implied.

Copy link
Contributor

aem-code-sync bot commented Nov 10, 2023

Page Scores Audits Google
/drafts/mathuria/masonry/poc/poc-2?martech=off PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI

@aishwaryamathuria
Copy link
Contributor Author

ould be like calling a product, "iPhone Basic" or "MacBoo

@auniverseaway I have made the required changes for suggestion. The block name is now be 'brick' in place of 'basic-brick'

@aishwaryamathuria
Copy link
Contributor Author

aishwaryamathuria commented Dec 5, 2023

@auniverseaway @rgclayton @ryanmparrish
We have made the suggested change as mentioned in #1515 (comment) in the PR. Section metadata would now directly control the size and layout and it would not need to be specified at the brick level.
Author can add new key 'masonry' and specify the div spans (ranging from 1 to 12) as required. The sizes can be added in multiple or single line as required.
Could you re-review the PR ?

Screenshot 2023-12-05 at 4 35 45 PM Screenshot 2023-12-05 at 4 46 37 PM

Reauthored documents
https://masonry--milo--aishwaryamathuria.hlx.page/drafts/mathuria/masonry/poc/poc-1
https://masonry--milo--aishwaryamathuria.hlx.page/drafts/mathuria/masonry/poc/poc-2

More documents available in Masonry folder

CC: @salonijain3 @dstrong23 @ccongerp

@rgclayton
Copy link
Contributor

2. I'm curious what the purpose of the new brick block is. Is it basically just the text block or does it have special functionality? The section CSS doesn't seem to need the children to be "bricks" to control the width, so just curious.

From my screenshot? If so, that was just a simple example of blocks. No new brick block. My main point revolved around the section metatdata

cc: @vivgoodrich

Copy link
Contributor

@rgclayton rgclayton left a comment

Choose a reason for hiding this comment

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

Adding a few comments.

libs/blocks/section-metadata/section-metadata.js Outdated Show resolved Hide resolved
libs/blocks/section-metadata/section-metadata.css Outdated Show resolved Hide resolved
@vgoodric
Copy link
Contributor

vgoodric commented Dec 5, 2023

  1. I'm curious what the purpose of the new brick block is. Is it basically just the text block or does it have special functionality? The section CSS doesn't seem to need the children to be "bricks" to control the width, so just curious.

From my screenshot? If so, that was just a simple example of blocks. No new brick block. My main point revolved around the section metatdata

cc: @vivgoodrich

When I look at files changed there is a js and CSS file for blocks/brick
image

@aishwaryamathuria
Copy link
Contributor Author

aishwaryamathuria commented Dec 5, 2023

  1. I'm curious what the purpose of the new brick block is. Is it basically just the text block or does it have special functionality? The section CSS doesn't seem to need the children to be "bricks" to control the width, so just curious.

From my screenshot? If so, that was just a simple example of blocks. No new brick block. My main point revolved around the section metatdata
cc: @vivgoodrich

When I look at files changed there is a js and CSS file for blocks/brick image

  1. I'm curious what the purpose of the new brick block is. Is it basically just the text block or does it have special functionality? The section CSS doesn't seem to need the children to be "bricks" to control the width, so just curious.

From my screenshot? If so, that was just a simple example of blocks. No new brick block. My main point revolved around the section metatdata
cc: @vivgoodrich

When I look at files changed there is a js and CSS file for blocks/brick image

@vgoodric The brick block is not similar to text block. It is a new block that has custom requirements (requirements listed in MWPW-137717) . And it can be arranged via masonry layout in multiple row and column format. Also the section changes are not tied to brick block and section (like in its current state) continues to operate independent of the same .

@vgoodric
Copy link
Contributor

vgoodric commented Dec 5, 2023

So I feel bad because it looks like the work to move sizing information off of the bricks and into the section metadata has already been completed. But I checked with Mimi and Christine and we don't want to use this system on homepage. To be fair, if you had content where bricks were designed to be interchangeably used at multiple sizes, I see the advantage to this new system. But all of our blocks only look right at the specific size they were built for. So being able to look at a brick's source and know what size it's supposed to be to look right is really helpful. And then everywhere we put the brick, it's automatically the right size.

As the 2 that have spent the most time building and authoring content in a masonry layout, I put a lot of stock in Christine and Mimi's opinion. And we feel strongly that we don't want to lose the option to just put the size right on the brick (and have a default size so most bricks don't need it). But maybe we should open it up to some author feedback? Maybe we want to support both?

@auniverseaway @aishwaryamathuria

@rgclayton
Copy link
Contributor

So I feel bad because it looks like the work to move sizing information off of the bricks and into the section metadata has already been completed. But I checked with Mimi and Christine and we don't want to use this system on homepage. To be fair, if you had content where bricks were designed to be interchangeably used at multiple sizes, I see the advantage to this new system. But all of our blocks only look right at the specific size they were built for. So being able to look at a brick's source and know what size it's supposed to be to look right is really helpful. And then everywhere we put the brick, it's automatically the right size.

As the 2 that have spent the most time building and authoring content in a masonry layout, I put a lot of stock in Christine and Mimi's opinion. And we feel strongly that we don't want to lose the option to just put the size right on the brick (and have a default size so most bricks don't need it). But maybe we should open it up to some author feedback? Maybe we want to support both?

@auniverseaway @aishwaryamathuria

My 2 cents.

@vivgoodrich Can the homepage not continue to operate with it's current set up once this is merged into Milo?

It makes sense to have the homepage tailored to a more specific set up since it's the homepage. It being added to the Milo repo, with a broader consumer base, does that specificity still make sense? Or does it make sense to extend it for more use cases?

@vgoodric
Copy link
Contributor

vgoodric commented Dec 6, 2023

My 2 cents.

@vivgoodrich Can the homepage not continue to operate with it's current set up once this is merged into Milo?

It makes sense to have the homepage tailored to a more specific set up since it's the homepage. It being added to the Milo repo, with a broader consumer base, does that specificity still make sense? Or does it make sense to extend it for more use cases?

Absolutely. If you guys decide not to support sizing on the bricks we will just not switch over and continue to do our own thing. But I don't think the viewpoints should be discounted as homepage only. I suspect that most blocks in a masonry design will be designed for a specific size and will not look good in others. I tried to view the figma doc in Jira but can't get it to work so maybe I just don't know how it will be used on the rest of the site. But in general if a brick is using a background image, it will likely look awful at the wrong size.

@spadmasa spadmasa added verified PR has been E2E tested by a reviewer and removed needs-verification PR requires E2E testing by a reviewer labels Dec 6, 2023
@auniverseaway
Copy link
Member

I think @vivgoodrich raises good points for masonry.

But all of our blocks only look right at the specific size they were built for.

That seems counter-intuitive to responsive web design. I would expect a block should look relatively correct at all sizes. We had this problem with HelpX where they built blocks "at specific sizes" and that makes the authoring brittle.

being able to look at a brick's source and know what size it's supposed to be to look right is really helpful

I think this is a great counterpoint. I don't know that I agree it's better than sections determining layout (like they should always do), but I totally get why someone might want this. You run the risk of putting two 2/3rd blocks next to each other and you won't realize it until A) you scroll, B) you preview.

Absolutely. If you guys decide not to support sizing on the bricks we will just not switch over and continue to do our own thing.

I think we should do this. I do not want these homepage patterns encouraged to the larger milo community.

This is the guidance we give to developers when faced with these questions. It does not specifically call out, "blocks should not size themselves." but hopefully it is implied with the fact that sections should own positioning of children.

Section Rules

Copy link
Contributor

@rgclayton rgclayton left a comment

Choose a reason for hiding this comment

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

I feel like we have a pretty good authoring pattern now with this.

Copy link
Contributor

@ryanmparrish ryanmparrish left a comment

Choose a reason for hiding this comment

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

Thanks for all the re-working on this.
I think it's looking good. Approved, but can you maybe take a quick look at the stylelint on your brick.css. I'm seeing a few linting issues. (see suggestion below)
My hope is we can expand on this in the future to possibly offer the best of both worlds w/ how HP is being authored.

.brick {
  position: relative;
  display: flex;
  text-size-adjust: none;
  min-height: 300px;
}

.brick,
.brick.click a.foreground {
  color: inherit;
}

.brick.click > a {
  text-decoration: none;
}

.brick.light,
.brick.light.click a.foreground {
  color: var(--text-color);
}

.brick.dark,
.dark.brick.click a.foreground {
  color: var(--color-white);
}

.brick .icon-stack-area li,
.brick .icon-stack-area li a {
  display: flex;
  align-items: center;
  gap: var(--spacing-xs);
  text-align: left;
}

.brick .foreground a:not([class]),
.brick .foreground span.first-link {
  font-weight: 700;
}

.brick .background {
  position: absolute;
  bottom: 0;
  left: 0;
  right: 0;
  top: 0;
  overflow: hidden;
}

.brick .foreground {
  position: relative; 
  display: flex;
  flex-grow: 1;
  padding: var(--spacing-m);
}

.brick.rounded-corners,
.brick.rounded-corners .background,
.brick.rounded-corners .foreground {
  border-radius: var(--spacing-xs);
}

.brick.align-center .foreground,
.brick.align-center .foreground .action-area,
.brick.center .foreground,
.brick.center .foreground .action-area {
  align-items: center;
  text-align: center;
  justify-content: center;
}

.brick.center .foreground {
  align-items: flex-start;
}

.brick .background div,
.brick .background p,
.brick .background picture {
  height: 100%;
  margin: 0;
  padding: 0;
}

.brick .background p,
.brick .background picture {
  display: block;
}

.brick .background img {
  object-fit: contain;
  object-position: bottom center;
  width: 100%;
  height: 100%;
}

.brick .mobile-only,
.brick .tablet-only,
.brick .desktop-only {
  display: none;
}

.brick .icon-stack-area li picture {
  display: flex;
  margin: 0;
  padding: 0;
  flex-shrink: 0;
}

.brick .foreground .icon-area picture {
  display: flex;
}

.brick .foreground p {
  padding: 0;
  margin: 0;
}

.brick .foreground div > * {
  margin-top: var(--spacing-xs);
}

.brick .foreground p:first-child,
.brick .foreground p.icon-area,
.brick .foreground p.icon-area + p {
  margin-top: 0;
}

.brick .foreground p.icon-area {
  display: inline-block;
  margin-bottom: var(--spacing-s);
}

.brick .foreground p.action-area {
  display: flex;
  flex-wrap: wrap;
  gap: 24px;
  margin-top: var(--spacing-s);
}

.brick .icon-stack-area li img {
  width: var(--icon-size-s);
  height: auto;
}

.brick .foreground .icon-area img {
  height: var(--icon-size-l);
  width: auto;
}

.brick .icon-stack-area {
  display: flex;
  flex-flow: row wrap;
  flex-direction: column;
  gap: var(--spacing-xs);
  width: 100%;
  margin: 0;
  padding: 0;
  list-style-type: none;
}

.brick.center .icon-stack-area,
.brick.align-center .icon-stack-area {
  display: inline-flex;
  width: auto;
}

[dir="rtl"] .brick .icon-stack-area li,
[dir="rtl"] .brick .icon-stack-area li a {
  text-align: right;
}

.brick.click a.foreground .first-link:not([class*="button"]) {
  color: var(--link-color);
  text-decoration: none;
}

.brick.click:hover a.foreground .first-link:not([class*="button"]) {
  text-decoration: underline;
  color: var(--link-hover-color);
}

.static-links .brick.click a.foreground .first-link:not([class*="button"]),
.static-links .brick.click a.foreground a:not([class*="button"]),
.brick.static-links.click a.foreground .first-link:not([class*="button"]),
.brick.static-links.click a.foreground a:not([class*="button"]) {
  color: inherit;
  text-decoration: underline;
}

.brick.click:hover .first-link.con-button.blue,
.brick.click:active .first-link.con-button.blue {
  background: var(--color-accent-hover);
  border-color: var(--color-accent-hover);
  color: var(--color-white);
}

.brick.click:hover .first-link.con-button,
.brick.click:active .first-link.con-button,
.brick.light.click:hover .first-link.con-button,
.brick.light.click:active .first-link.con-button,
.light .brick.click:hover .first-link.con-button,
.light .brick.click:active .first-link.con-button {
  background-color: var(--color-black);
  border-color: var(--color-black);
  color: var(--color-white);
}

.dark .brick.click:hover .first-link.con-button,
.brick.dark.click:active .first-link.con-button {
  background-color: var(--color-white);
  color: var(--color-black);
  text-decoration: none;
}


@media screen and (max-width: 600px) {
  .brick .mobile-only {
    display: block;
  }
}

@media screen and (min-width: 600px) {
  .brick {
    min-height: 384px;
  }

  .brick .foreground {
    padding: var(--spacing-l);
  }
}

@media screen and (min-width: 600px) and (max-width: 1199px) {
  .brick .tablet-only {
    display: block;
  }
}

@media screen and (min-width: 1200px) {
  .brick .desktop-only {
    display: block;
  }

  .brick.large {
    min-height: 500px;
  }
}

@aishwaryamathuria
Copy link
Contributor Author

he future to possibly offer the best of both worlds

@ryanmparrish I have made the suggested changes

@salonijain3 salonijain3 merged commit 951817c into adobecom:main Dec 7, 2023
11 checks passed
VKniaz pushed a commit to VKniaz/milo that referenced this pull request Dec 15, 2023
* Basic brick and masonry block block

* latest update

* Cleanup

* Cleanup

* Masonry brick height

* Unit tests

* Cleanup

* Cleanup

* CHanginf block name

* Target non empty p tags only

* Reduce line of code for button

* Fixing font-weight

* Fixing font-weight

* Review comments

* Fixing object fit issue

* Adding brick block

* Filter the p tag to remove empty and whitespace ones

* MWPW-139392

* MWPW-139392

* MWPW-139392

* MWPW-139812

* Tmp change

* revert Tmp change

* Review comments

* Review comments

* Unit tests

* Fix

* Fix

* Fix

* Fix

* Fix

* Align bricks

* Unit tests

* Remove extra span 12

* Update class name

* fixing lint error

---------

Co-authored-by: Blaine Gunn <[email protected]>
vgoodric pushed a commit to vgoodric/bootcamp-milo that referenced this pull request Feb 1, 2024
* Basic brick and masonry block block

* latest update

* Cleanup

* Cleanup

* Masonry brick height

* Unit tests

* Cleanup

* Cleanup

* CHanginf block name

* Target non empty p tags only

* Reduce line of code for button

* Fixing font-weight

* Fixing font-weight

* Review comments

* Fixing object fit issue

* Adding brick block

* Filter the p tag to remove empty and whitespace ones

* MWPW-139392

* MWPW-139392

* MWPW-139392

* MWPW-139812

* Tmp change

* revert Tmp change

* Review comments

* Review comments

* Unit tests

* Fix

* Fix

* Fix

* Fix

* Fix

* Align bricks

* Unit tests

* Remove extra span 12

* Update class name

* fixing lint error

---------

Co-authored-by: Blaine Gunn <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-feature New block or other feature run-nala Run Nala Test Automation against PR verified PR has been E2E tested by a reviewer
Projects
None yet
Development

Successfully merging this pull request may close these issues.