Skip to content
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

feat(breadcrumb): add the breadcrumb component #1260

Merged
merged 30 commits into from May 8, 2022
Merged

Conversation

YonatanKra
Copy link
Contributor

@YonatanKra YonatanKra commented Mar 18, 2022

Adds the breadcrumb and breadcrumb-item components.
Note that this is "quick and dirty" as vivid-2 is deprecated. No unit tests involved - mainly copy-paste of code from vivid-3

@github-actions
Copy link

🚀

Latest successful build of the PR deployed here.

🚀

},
"dependencies": {
"@vonage/vvd-foundation": "^2.26.0",
"lit-element": "^2.4.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing core

Copy link
Contributor Author

Choose a reason for hiding this comment

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

vvd-core?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes it should be imported in all vivid < 3 components

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually - it doesn't use any of these...
I've removed v-2 deps. Left only umbrella for build time dep.

components/breadcrumb/package.json Outdated Show resolved Hide resolved
components/breadcrumb/package.json Outdated Show resolved Hide resolved
__snapshots__/snackbar.md Show resolved Hide resolved
Comment on lines 5 to 11
const style = document.createElement('style');
style.innerHTML = `
vwc-breadcrumb {
--icon-size: 14px;
}
`;
document.head.appendChild(style);
Copy link
Contributor

Choose a reason for hiding this comment

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

why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to set the icon's size inside the element.
Since we have no access to the scss file and actually have no scss file, this is the way I thought of doing it.
Got a suggestion?

components/breadcrumb/package.json Outdated Show resolved Hide resolved
yinonov
yinonov previously approved these changes May 3, 2022
yinonov
yinonov previously approved these changes May 6, 2022
@YonatanKra YonatanKra force-pushed the breadcrumb-component branch 2 times, most recently from b0898d1 to 71381db Compare May 6, 2022 11:18
@sonarcloud
Copy link

sonarcloud bot commented May 8, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 8 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

},
"scripts": {
"build:typescript": "tsc -b",
"build:styles": "umbrella-style-modules --cssLib '@microsoft/fast-element'",
Copy link
Contributor

Choose a reason for hiding this comment

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

ok got it

</vwc-breadcrumb>`;

export const Basic = Template.bind({});
Basic.args = { label: 'Basic', disabled: 'false' };
Copy link
Contributor

Choose a reason for hiding this comment

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

there's no use of either of them in the code - I think you better remove them, less confusion to our users :)

@YonatanKra YonatanKra merged commit bf0c5ed into master May 8, 2022
@yinonov yinonov deleted the breadcrumb-component branch May 8, 2022 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants