header component implementation and examples #22
Conversation
| @property({ type: String, reflect: true }) template: 'basic' | 'singleRow' | 'separateNavigation' = 'basic'; | ||
|
|
||
| render() { | ||
| const isRtl = this.localize.dir() === 'rtl'; |
There was a problem hiding this comment.
we should see if we can control if this code runs on the server or the browser to enable server side rendering
idea would be something like this
let isRtl = false;
if (inBrowser) {
isRtl = this.localize.dir() === 'rtl';
}There was a problem hiding this comment.
suggestion here is to adjust the localizeController to adapt to rtl params sent from the server on SSR, and not force inBrowser checks for the components, or we add a different controller to evaluate browser or server
| export default class MfHeader extends ShoelaceElement { | ||
| static styles: CSSResultGroup = [componentStyles, styles]; | ||
|
|
||
| private readonly localize = new LocalizeController(this); |
There was a problem hiding this comment.
this should be initialized only if we run this code in browser, localize should be LocalizeController | null
There was a problem hiding this comment.
similar to previous comments
| const isRtl = this.localize.dir() === 'rtl'; | ||
|
|
||
| return html` | ||
| <header |
There was a problem hiding this comment.
Are we forcing html <header for this component?
In general I would not override html elements and if we do we should highlight on the documentation.
This will impact the demo page.
I think for this component has sense to use <header> so let's just add this into the doc
abu-seini-maf
force-pushed
the
feat/new-mf-header
branch
from
f99cf5b to
c988ba4
Compare
abu-seini-maf
force-pushed
the
feat/new-mf-header
branch
from
c988ba4 to
2c57519
Compare
|
LGTM and merging |
| ```html:preview | ||
| <mf-header style="max-width: 1400px;margin: 0 auto;" template="separateNavigation"> | ||
| <mf-img slot="logo" | ||
| src="https://api-prod.thatconceptstore.com/medias/THATlogo.svg?context=bWFzdGVyfGltYWdlc3wxOTA1fGltYWdlL3N2Zyt4bWx8YURBNEwyZ3pPQzg1TkRNNE5EUTVNak00TURRMkwxUklRVlJzYjJkdkxuTjJad3xkMjhkOGMyODljNjEyMzlhODBkZDQzMGM0NDdiNzEzODljNjU0NWU1MmJlZDk3MjVkYTA5Y2Y1NDBiZGU1MzJi" |
There was a problem hiding this comment.
It’s better to use local images instead of linking to external resources to avoid potential issues with broken links or cross-site problems in the future. Please consider downloading the image and updating the src to point to a local path within the project.
|
|
||
| <mf-img slot="logo" src="https://www.dev.mallgiftcard.ae/assets/maf-logo-header.svg" alt="MAF"></mf-img> | ||
|
|
||
| <mf-navigation slot="navigation"> |
| <mf-nav-item> | ||
| Neighbourhoods | ||
| <mf-navigation slot="subnav" vertical="true"> | ||
| <mf-nav-item value="find">Serra</mf-nav-item> |
There was a problem hiding this comment.
aria-label to be added for each nav item
No description provided.