feat (esco-content-menu) : Lit version of the component #919
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
jgribonvald
changed the title
jgribonvald
force-pushed
the
feat-esco-content-menu-lit
branch
6 times, most recently
from
c8f257b
to
7e8821b
Compare
gbrousse-recia
force-pushed
the
feat-esco-content-menu-lit
branch
from
f09d702
to
bfd2b17
Compare
There was a problem hiding this comment.
@bjagg @ChristianMurphy @cbeach47 do you have feedback around this change ? We need to remove old browsers, and IE support should go out !
There was a problem hiding this comment.
Thanks @jgribonvald ! It's impressive - I haven't add a chance to run this locally yet, or dig too deep in the code, but added some inline comments.
For the readme, I would do a doc-wide find/replace for Emited/emited and replace with Emitted/Emitted .
|
|
||
| - `messages`: type: `Array of Objects`, see [internationalization](#internationalization) | ||
| - `context-api-url`: type: `String`, default: `/uPortal`, usefull to provide a different uPortal context on which to do request | ||
| - `favorite-api-url`: type: `String`, default: `/uPortal/api/layout`, the uri/url of the favorites api |
There was a problem hiding this comment.
Nice that this is separated out - /api/portlet-list is in the works!
There was a problem hiding this comment.
@cbeach47 Yes but there is a breaking change, the ordering isn't managed by code and I think that the json format isn't the same... And in waiting an improvment the ordering is assured by adding always at the end of the list the new favorite, hibernate keep the ordering from list and your new code add this param.
|
|
||
| ### The content favorites | ||
|
|
||
| The component is functional only into the `content-menu`, it needs that a portlet list and favorite list is passed |
There was a problem hiding this comment.
What is 'portlet list', ie which api does it come from?
There was a problem hiding this comment.
The portlet list is only an array passed to the component, so it's resolved from a parent component and using a service class that resolve from the current portlet api system.
gbrousse-recia
force-pushed
the
feat-esco-content-menu-lit
branch
3 times, most recently
from
8efbbc9
to
b74c325
Compare
There was a problem hiding this comment.
Welcome @gbrousse-recia! Thank you for looking into this!
Broadly I'm excited to move away from the deprecated version on Vue and on to lit.
A few points of discussion towards the approach below.
| # Editor configuration, see http://editorconfig.org | ||
| root = true | ||
|
|
||
| [*] | ||
| charset = utf-8 | ||
| indent_style = space | ||
| indent_size = 2 | ||
| insert_final_newline = true | ||
| trim_trailing_whitespace = true |
There was a problem hiding this comment.
is this needed with prettier also being used?
There was a problem hiding this comment.
I think it's a good complement to prettier and eslint, allowing all developers to have the same configuration in their ide
| "sass-loader": "^13.0.0", | ||
| "terser-webpack-plugin": "^5.3.3", | ||
| "ts-loader": "^9.3.0", | ||
| "typescript": "^4.7.3", |
There was a problem hiding this comment.
Happy to see typescript integrated to ensure variables are typed and APIs/interfaces are clearly defined 👍
| "terser-webpack-plugin": "^5.3.3", | ||
| "ts-loader": "^9.3.0", | ||
| "typescript": "^4.7.3", | ||
| "webpack": "^5.73.0", |
There was a problem hiding this comment.
I'm a bit cautious using webpack here.
Vue CLI is/was also webpack based, and even through it provided good defaults, it was still a pain to maintain.
The deep dependency tree frequently has vulns which cause noise as dependabot and renovate bot try to respond.
WebPack had (and may still have) issues around handling code in ESM format.
And when custom configuration is needed, it can be rather verbose and circuitous.
While not a blocker.
I'd lean strongly towards moving to another build system like:
- Rollup (https://lit.dev/docs/tools/production/#building-with-rollup)
- Vite (https://vitejs.dev/guide/) ⭐
- ESBuild (https://github.com/zandaqo/esbuild-plugin-lit#readme)
- Parcel (https://parceljs.org/)
| return false; | ||
| } | ||
|
|
||
| willUpdate(changedProperties: Map<string | number | symbol, unknown>): void { |
There was a problem hiding this comment.
Another area which could be fine here.
But may want to be discussed further.
Does it make sense to stick with lifecycle based design for components?
The hooks based model has taken both React and Vue over, and can be supported in Lit as well https://github.com/matthewp/haunted#readme
There was a problem hiding this comment.
regarding 3256431, we probably don't want to commit dist files/folders?
Those are generally only sent to a package registry like npm or GitHub packages.
Keeping them in source control introduces risk that the wrong file could be edited, and the change could be overriden/not applied.
gbrousse-recia
force-pushed
the
feat-esco-content-menu-lit
branch
from
3256431
to
ae3dc87
Compare
gbrousse-recia
force-pushed
the
feat-esco-content-menu-lit
branch
from
36c7dbc
to
0870f3b
Compare
gbrousse-recia
force-pushed
the
feat-esco-content-menu-lit
branch
from
f08b7bc
to
ec4381a
Compare
gbrousse-recia
force-pushed
the
feat-esco-content-menu-lit
branch
from
a683a5a
to
00c2f5e
Compare
Lit version of the esco-content-menu component