Skip to content
This repository has been archived by the owner on May 2, 2023. It is now read-only.

feat(menu-legacy): menu-legacy twig implementation - TWIG-5 #119

Merged
merged 7 commits into from
Sep 6, 2019

Conversation

serotonine
Copy link
Contributor

PR description

Please drop a few lines about the PR: what it does, how to test it, etc.

QA Checklist

In order to ensure a safe and quick review, please check that your PR follow those guidelines:

  • I have put the vanilla component as devDependencies
  • I have put the specs package as devDependencies
  • I have added the components directly used in the twig file (with include or embed) as dependencies
  • My component is listed in @ecl-twig/ec-components's dependencies
  • My variables naming follow the guidelines (snake case for twig)
  • I have provided tests
  • I have provided documentation (for the "notes" tab)
  • If my local yarn.lock contains changes, I have committed it
  • I have given my PR the proper label (pr: review needed to indicate that I'm done and now waiting for a review ,pr: wip to indicate that I'm actively working on it ...)

@emeryro emeryro self-assigned this Sep 3, 2019
Copy link
Contributor

@emeryro emeryro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there are a few things to be fixed, but overall the component looks nice, good job!

package.json Outdated
@@ -13,7 +13,7 @@
"postinstall": "lerna run postinstall && lerna run prepublish",
"pretty-check": "prettier --check \"src/**/*.{md,mdx,html,json,yml,js}\"",
"publish": "scripts/publish.sh",
"start:ec": "start-storybook -p 9001 -c ./src/ec/.storybook",
"start:ec": "start-storybook -p 8082 -c ./src/ec/.storybook",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a reason why you changed this port? This may affect other people working on the project too.

Copy link
Contributor Author

@serotonine serotonine Sep 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason is I'm working on Cloud9 and there is no port 9001 on it.
but this file does not be pushed.
I fix it then maybe it's better I add this file in gitignore.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you have to change the port, that's not really an issue, but you just have to syncronize with other people working on the project to ensure that it won't cause trouble.
I don't recommand to put this file in gitignore anyways.

@@ -0,0 +1,46 @@
# ECL Twig - EC Navidation Menu

npm package: `@ecl-twig/ec-component-navigation-menu`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

component name should be updated here (and next line too)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok. I also write the right example...


Parameters:

- "title" (string) (default: '')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

parameters do not seems to be up to date (here and in example below)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done :-)

"children" (array) (default: []): [{
"items" (array) (default: [{}]): [{
- "label" (string): label of the link
- "href" (string): target of the link
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

children items can have a isCurrent parameter too

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

<nav data-ecl-auto-init="MenuLegacy" class="{{ _css_class }}" data-ecl-menu-legacy="true" aria-expanded="false" {{ _extra_attributes|raw }}>
<div class="ecl-container">
<button class="{{ _prefix_class }}__toggle {{ _prefix_class }}__hamburger ecl-button ecl-button--ghost" type="button" data-ecl-menu-legacy-hamburger-button="true" >
<div class="ecl-menu-legacy__toggle-container">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these classes name should use _prefix_class to be consistent (that's the same for some other classes in this file)
or you don't use a prefix at all, it's up to you :)

@emeryro emeryro removed their assignment Sep 3, 2019
@serotonine serotonine added pr: review needed Use this label to show that your PR needs to be review and removed pr: modification needed labels Sep 4, 2019
@emeryro emeryro self-assigned this Sep 5, 2019
@emeryro emeryro added pr: under review and removed pr: review needed Use this label to show that your PR needs to be review labels Sep 5, 2019
@emeryro
Copy link
Contributor

emeryro commented Sep 5, 2019

The component is good.
Please just update the jest snapshots for menu legacy (see drone report) and we could merge it.

@emeryro emeryro removed their assignment Sep 5, 2019
@serotonine serotonine added pr: review needed Use this label to show that your PR needs to be review and removed pr: modification needed labels Sep 5, 2019
@emeryro emeryro merged commit 550b48f into v2.9-dev Sep 6, 2019
@emeryro emeryro deleted the TWIG-5 branch September 6, 2019 10:59
@emeryro emeryro removed the pr: review needed Use this label to show that your PR needs to be review label Sep 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants