feat(bc-footer): Add Brand.com Footer #1450
Conversation
|
Test summary
Run details
View run in Cypress Dashboard ➡️ This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard |
Guilherme-Almeida-Zeni
force-pushed
the
feature/bc-footer
branch
2 times, most recently
from
9594c1e
to
b9abb10
Compare
Guilherme-Almeida-Zeni
force-pushed
the
feature/bc-footer
branch
from
b9abb10
to
4ef4a43
Compare
packages/__cxstarter__/src/components/GlobalFooter/tokens/__cxstarter__GlobalFooter.ts
Outdated
Show resolved
Hide resolved
packages/cx-layout/src/components/FooterMenu/FooterMenu.token.ts
Outdated
Show resolved
Hide resolved
packages/cx-layout/src/components/GlobalFooter/SocialLinks/SocialLinksClean.tsx
Outdated
Show resolved
Hide resolved
..._/src/data/site/FooterMenus$menu-item$10e8714a-3a9e-4025-a8dc-f7465c4a2a57$cham-sublist.json
Outdated
Show resolved
Hide resolved
packages/cx-layout/src/components/GlobalFooter/Rewards/index.ts
Outdated
Show resolved
Hide resolved
| <C.Wrapper> | ||
| <C.Container> | ||
| <C.SectionTop> | ||
| <C.Rewards /> |
There was a problem hiding this comment.
Should we have wrappers for these? RewardsWrapper, MenuWrapper, etc? cc @hvanyo
There was a problem hiding this comment.
@wodenx I think general rule is Wrap all slots for flexibility. if we don't use the wrapper slot then set it to fragment.
There was a problem hiding this comment.
Actually, the components Rewards, Social Links, and FooterMenus, already have a wrapper:
Still, do you think the wrappers should be added in the FooterCleanBase component, instead of in the components themselves?
For Copyright, I don't see a reason why it should have a wrapper... except for Penaten, which has the copyright made in different way, and for Zarbess, which has very customized copyright, I don't see wrappers for it in the other sites, or even in CanvasX/Bodiless. Is there any specific reason why we should add an wrapper for just text?
There was a problem hiding this comment.
I want to add wrappers at least for any component which we expect to receive an editor (whether that is a flow container, plain text, or rich text). The reason is that it makes it easier to apply props to the editor (like a placeholder or nodeKey) without reaching in via withDesign as you'd have to do if we used 'withEditor...'. Also makes it easier to swap out the editor without changing the formatting applied to the wrapper.
This is already the stndard pattern for content areas which take flow containers, just want to make it consistent for other editors.
I suppose an alternative would be to have all our editors provide their own wrappers... We can discuss at standup today.
There was a problem hiding this comment.
For all those components which "already hve a wrapper" -- we should pass props from the top-level "clean" component to the wrapper to faciltate layout styling in a parent component.
There was a problem hiding this comment.
@wodenx ,
As aligned in the standup, added wrappers to all the components and updated styles to reflect it. Please, review it again, thanks.
packages/cx-layout/src/components/GlobalFooter/GlobalFooterClean.tsx
Outdated
Show resolved
Hide resolved
packages/cx-layout/src/components/GlobalFooter/cxGlobalFooter.ts
Outdated
Show resolved
Hide resolved
packages/cx-layout/src/components/GlobalFooter/cxGlobalFooter.ts
Outdated
Show resolved
Hide resolved
| const Base = asGlobalFooterToken({ | ||
| Components: { | ||
| Wrapper: startWith(Div), | ||
| Rewards: flowHoc( |
There was a problem hiding this comment.
Components domain should not have anything except tokens from other components. I think all the stylin gon the wrapper here shoudl go into a cxRewards token (why isn' tit in Default, anyway?)
There was a problem hiding this comment.
if this styling is not appropriate for cxRewards.default, then it should go into other domains (layout, spacing). And, we should either pass the component props to the wrapper (in cxRewards), or have a RewardsWrapper slot here. @hvanyo thoughts on passing clean component props to its outer wrapper, eg:
const MyComponentClean = ({ components: C, ...rest }) -> (
<C.Wrapper {...rest}>
...
```
There was a problem hiding this comment.
@wodenx ,
IMO, that style shouldn't be in the cxRewards Default, since it seems more something for the slots design than for the rewards component itself. I mean, suppose we want to reuse the rewards component somewhere else than in the footer (in a page for example), that spacing/layout will not probably make sense.
The same works in the other way, suppose we have to replace the Rewards with another component, I can just switch the component in the slot (which makes me wonder if we should have the slot names as Rewards) and the new component would be in the same place without rewriting the same style inside the component.
Well, right now, since it seems we are waiting @hvanyo to answer about passing the component props to the wrapper, I just updated styling to be in the proper domains (layout, spacing).
Please, take a look if that's okay for now. Thanks!
There was a problem hiding this comment.
@wodenx ,
As aligned in the standup, applied wrapper in the slots instead of using props.
|
|
||
| import { withNode, withNodeKey } from '@bodiless/core'; | ||
| import { cxColor, cxFontSize } from '@bodiless/cx-elements'; | ||
| import { withEditorFull } from '@bodiless/cx-editors'; |
There was a problem hiding this comment.
I'd like to avoid withEditor... and instead provide slots in the top level component for editors. cc @hvanyo
There was a problem hiding this comment.
we need to start righting the best practices down
There was a problem hiding this comment.
| }, | ||
| Schema: { | ||
| FooterMenus: flowHoc( | ||
| withNode, |
There was a problem hiding this comment.
do we need these calls to withNode?
There was a problem hiding this comment.
Hello @wodenx ,
I think we do. Maybe you have another idea, but just for you to understand, if I just provide the withNodeKey, the data is saved inside the pages directory, instead of the site directory. Also, without the withNode, the file names don't contain the key FooterMenus. Is that okay to keep it then?
There was a problem hiding this comment.
ok. but i would like to wrap th ewhole footer in a node and give it a "footer" node key - then the menus and other compoennts inside will be automatically namespaced, so the "FooterMenu" can just be "menu".
I;d also like to avoid mixed-case node keys, to avoid possible problems on systems with different casing.
There was a problem hiding this comment.
@wodenx ,
Updated "FooterMenu" key to "footer". Also, did the same for "Copyright" (turned into "copyright").
packages/cx-layout/src/components/GlobalFooter/Rewards/index.ts
Outdated
Show resolved
Hide resolved
| Theme: { | ||
| Wrapper: cxColor.BgSecondaryFooter, | ||
| }, | ||
| Editors: { |
There was a problem hiding this comment.
So if we hvae a then this would just be (cxEditors.Copyright) or, if we don't want to make that a global RichText token, it would be cxFooterEditors.Copyright (a token defined somewhere in this package)
There was a problem hiding this comment.
to me it's a question of where we want it to appear in the styleguide - do we want the "editors" page of the styleguide to have a "Copyright" token which can be appied to show how the editor looks for copyrigth? cc @hvanyo
There was a problem hiding this comment.
Addreess. Please, review it again.
packages/cx-layout/src/components/GlobalFooter/SocialLinks/SocialLinksClean.tsx
Outdated
Show resolved
Hide resolved
Guilherme-Almeida-Zeni
force-pushed
the
feature/bc-footer
branch
from
591056a
to
b13f2be
Compare
| @@ -0,0 +1,102 @@ | |||
| /** | |||
There was a problem hiding this comment.
Needs to follow standard structure:
components/Footer/
FooterClean.tsx
index.ts. # imports default from tokens and exports as cxFooter
tokens/
cxFooter.ts. # default export is token collection
index.ts # just re-imports and re-exports default export
There was a problem hiding this comment.
Then we can add static tokens.
There was a problem hiding this comment.
@wodenx ,
This has been addressed. Please, review it again, thanks!
Guilherme-Almeida-Zeni
force-pushed
the
feature/bc-footer
branch
from
38c492d
to
6554a8d
Compare
Guilherme-Almeida-Zeni
force-pushed
the
feature/bc-footer
branch
from
6554a8d
to
e2b1886
Compare
| paragraph: as( | ||
| cxColor.TextPrimaryFooterCopy, | ||
| cxFontSize.XS, | ||
| 'border-white-400 border-t border-b md:border-0 lg:text-m-xs', |
There was a problem hiding this comment.
| 'border-white-400 border-t border-b md:border-0 lg:text-m-xs', | |
| 'border-white-400 border-t border-b md:border-0', |
There was a problem hiding this comment.
and it looks this copyright editor is also getting cxFontSize.BaseSize from original RTE we need to remove.
Same thing is happening for Links
There was a problem hiding this comment.
This has been addressed in this PR: Guilherme-Almeida-Zeni#10
| cxColor.TextPrimaryFooterCopy, | ||
| cxTextDecoration.Bold, | ||
| // @todo should we use tokens here? | ||
| 'text-m-xl md:text-m-lg lg:text-base', |
There was a problem hiding this comment.
@guilhermegpessoa
Please convert these to use tokens inscope of this ticket.
| /** | ||
| * An element token which adds a separator border to a footer menu item. | ||
| */ | ||
| const FooterMenu = asElementToken({ |
There was a problem hiding this comment.
@guilhermegpessoa This should be named Separator and not FooterMenu
| Theme: { | ||
| Title: as( | ||
| cxColor.TextPrimaryFooterCopy, | ||
| 'text-base', |
There was a problem hiding this comment.
@guilhermegpessoa please use tokens here.
Guilherme-Almeida-Zeni
changed the title
* bugfix(bc-footer): Fix Footer Issues * Remove top margin from in footnote * Add uppercase to submenu items * Removing unused imports * bugfix(bc-footer): Fix comments on PR * bugfix(bc-footer): Fix link issues Co-authored-by: Guilherme Zeni <gzeni@its.jnj.com> Co-authored-by: guilhermegpessoa <guilherme.gpessoa@hotmail.com>
…into feature/bc-footer
…into feature/bc-footer
* fix(bc-footer): Fix padding issues * fix(bc-footer) Fix padding values * fix(bc-footer): Fix padding issues
…into feature/bc-footer
…/Bodiless-JS into feature/bc-footer
* bugfix(bc-footer): Fix Footer Menu Accessibility Issues * bugfix(bc-footer): Add External And Download Icons To Menu Title Links Co-authored-by: Guilherme Zeni <gzeni@its.jnj.com>
Co-authored-by: Guilherme Zeni <gzeni@its.jnj.com>
Overview
Provide footer for brand.com.
Changes
bodiless/navigationas dependency for cx-layout package.TextPrimaryFooterCopy: 'text-cx-primary-footer-copy').FooterCleanandcxFooterto be used by page layout.Test Instructions
Footer can be found at
/typographyfrom cxstarter site:npm run setupcd sites/__cxstarter__npm run startRelated Issues