-
Notifications
You must be signed in to change notification settings - Fork 159
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
feat(link-list): aem improvements to end-of-section type #4972
feat(link-list): aem improvements to end-of-section type #4972
Conversation
Deploy preview created for package Built with commit: 00ffc5c2f89b8110665bef309fc6ca8a3c93bb39 |
Deploy preview created for package Built with commit: 00ffc5c2f89b8110665bef309fc6ca8a3c93bb39 |
Deploy preview created for package Built with commit: 00ffc5c2f89b8110665bef309fc6ca8a3c93bb39 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great progress @raphaelamadeu!
@property({ reflect: true }) | ||
type = LINK_LIST_ITEM_TYPE.DEFAULT; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a JSDoc comment that describes what this property is for? That will be reflected to docs page in Storybook.
* Should happen if there are more than 6 child items slotted in. | ||
*/ | ||
@internalProperty() | ||
private _useThreeColumnLayoutForEndType = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you rename _useSplitLayoutForEndType
with _endTypeLayout
? Good to define an internal enum (defined in this file) like END_TYPE_LAYOUT.TWO_COLUMNS
and END_TYPE_LAYOUT.THREE_COLUMNS
.
@@ -31,3 +31,18 @@ export enum LINK_LIST_TYPE { | |||
*/ | |||
END = 'end', | |||
} | |||
|
|||
/** | |||
* Link list item type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Link list item type | |
* Link list item type. Should mirror `LINK_LIST_TYPE` of parent `<dds-link-list>`. |
if (this.type === LINK_LIST_TYPE.END) { | ||
childItems.forEach(elem => { | ||
(elem as DDSLinkListItem).type = LINK_LIST_ITEM_TYPE.END; | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make sure updating type
of child <dds-link-list-item>
in updated()
lifecycle method, too, to cope with change in type
after <dds-link-list-item>
is slotted in.
.#{$dds-prefix}-ce--link-list__list--three-columns { | ||
@include carbon--breakpoint('sm') { | ||
::slotted(#{$dds-prefix}-link-list-item) { | ||
margin-right: -$carbon--layout-01; | ||
} | ||
} | ||
|
||
@include carbon--breakpoint('md') { | ||
display: grid; | ||
grid-template-columns: 1fr 1fr 1fr; | ||
grid-column-gap: 2rem; | ||
|
||
::slotted(#{$dds-prefix}-link-list-item) { | ||
display: block; | ||
margin-left: -$carbon--layout-01; | ||
margin-right: 0; | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There seem many duplicate CSS rules with two columns mode. One way to solve it is:
.#{$dds-prefix}-ce--link-list__list--split {
/* The common rules */
}
.#{$dds-prefix}-ce--link-list__list--two-columns {
/* 2-column mode-specific rules */
}
.#{$dds-prefix}-ce--link-list__list--three-columns {
/* 3-column mode-specific rules */
}
.#{$dds-prefix}-ce--link-list__list--split,
.#{$dds-prefix}-ce--link-list__list--three-columns {
/* The common rules */
}
@raphaelamadeu @StephanieHoman is the designer on this work and should be assigned to review this PR. Please update the design issue (#4900), letting her know that it is ready and provide the links where she can review it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍 - Thanks @raphaelamadeu for the update!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…gn-system#4972) ### Related Ticket(s) carbon-design-system#4902 ### Description End variant of Link List was updated to reflect the new design specs by AEM team. ![image](https://user-images.githubusercontent.com/30945011/105411394-21baf780-5c12-11eb-95b7-2c488694fc3e.png)
Related Ticket(s)
#4902
Description
End variant of Link List was updated to reflect the new design specs by AEM team.