-
Notifications
You must be signed in to change notification settings - Fork 25
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
providing some improvements on esco-content-menu #303
providing some improvements on esco-content-menu #303
Conversation
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.
Thanks @jgribonvald!
When you have a chance could you run npm run format
?
3d8cfc9
to
84df88b
Compare
Ok this time it's ok and validated by @vrepain (ESUP member) |
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.
@jgribonvald Nice improvements!
There are a couple merge conflicts, and converting the background boolean attribute to a css variable the are currently blocking this from being merged.
This is close to being merge-able, let me know if you have any questions on the remaining changes requested.
@ChristianMurphy : I have to watch so let me some times. |
89e2c79
to
0e20f95
Compare
I've just rebased to fix merge conflict, I didn't change the background option for now |
ef4a14b
to
a5287f6
Compare
@jgribonvald there is a merge conflict in |
Yes I will look on, I didn't see the conflict |
@ChristianMurphy This is rebased and the conflict resolved ! |
if no lang exist it will try to retrieve the lang from the browser lang
I fixed the favorite-content, when using the content-grid instead of the swipper the context-api and favorites-api parameter were missing to be able to customize the EDIT: for me all seems good now ! |
This provide some improvments :