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.

Sign up for GitHub

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

Jump to bottom

feat: add dashboard category accordion #288

Merged
merged 1 commit into from
Aug 11, 2023
Merged

feat: add dashboard category accordion #288

merged 1 commit into from
Aug 11, 2023

Conversation

kertuilves
Copy link

@kertuilves kertuilves commented Jul 5, 2023 •
edited
Loading

@kertuilves kertuilves requested a review from pawelkmpt July 5, 2023 06:30
@github-actions
Copy link

github-actions bot commented Jul 5, 2023 •
edited
Loading

size-limit report 📦

Path Size
packages/cxl-ui/pkg/dist-web/cxl-ui.js 34.59 KB (+1.94% 🔺)
packages/cxl-ui/pkg/dist-web/cxl-ui-jwplayer.js 11.87 KB (0%)
packages/cxl-ui/pkg/dist-web/cxl-ui-playbooks.js 25.68 KB (+1.57% 🔺)
packages/cxl-ui/pkg/dist-web/vendor.js 125.6 KB (0%)
packages/cxl-ui/pkg/dist-web/cxl-ui-jwplayer.js, packages/cxl-ui/pkg/dist-web/cxl-ui-playbooks.js, packages/cxl-ui/pkg/dist-web/cxl-ui.js, packages/cxl-ui/pkg/dist-web/manifest.js, packages/cxl-ui/pkg/dist-web/unresolved.js, packages/cxl-ui/pkg/dist-web/vendor.js 198.9 KB (+0.54% 🔺)

@pawelkmpt pawelkmpt requested review from lkraav and anoblet July 5, 2023 08:02
@heshfekry
Copy link

Hey @kertuilves

The progress bar is missing from the sub section.

image

Except for that great job on the component both on desktop and mobile i didn't see anything immediately in UX.

pawelkmpt
pawelkmpt approved these changes Jul 5, 2023
View reviewed changes
Copy link

@pawelkmpt pawelkmpt left a comment

Choose a reason for hiding this comment

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

LGTM. I just left some questions

packages/cxl-lumo-styles/scss/themes/vaadin-accordion-panel.scss Show resolved Hide resolved
packages/cxl-ui/scss/global/cxl-vaadin-accordion.scss Outdated Show resolved Hide resolved
@heshfekry
Copy link

image

A little bit of padding on the horizontal scroll indicator perhaps.

@pawelkmpt
Copy link

A little bit of padding on the horizontal scroll indicator perhaps.

Design does not have scrollbar. Maybe we'd like to hide it @heshfekry ?

@heshfekry
Copy link

@pawelkmpt hide works too. Netflix design also does not have it which is what its based of off.

@kertuilves kertuilves force-pushed the kertu/feat/dashboard-category-accordion branch from 8552bda to 9fb41ec Compare July 6, 2023 07:46
@pawelkmpt
Copy link

Is #289 dependency for this PR? Should we commit lightweight card so you can use here?

@pawelkmpt pawelkmpt changed the base branch from master to feat/dashboard July 6, 2023 08:05
anoblet
anoblet approved these changes Jul 7, 2023
View reviewed changes
Copy link
Collaborator

@anoblet anoblet left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Similar to what @pawelkmpt said, I would add #289 as a dependency in the PR description:

Clickup: https://app.clickup.com/t/861n0qkvd
Dependencies:

@kertuilves kertuilves force-pushed the kertu/feat/dashboard-category-accordion branch from 9fb41ec to efa005c Compare July 10, 2023 07:34
@kertuilves
Copy link
Author

Is #289 dependency for this PR? Should we commit lightweight card so you can use here?

I didn't see it would be necessary because it would just change the card content and it doesn't affect the layout. But if it's something that should be done I'm up for it and will do the changes.

pawelkmpt
pawelkmpt requested changes Jul 14, 2023
View reviewed changes
Copy link

@pawelkmpt pawelkmpt left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Please rebase on top of feat/dashboard and solve conflicts.

Besides that it looks good but I'd wait for #289 and use it here in storybook instead of current cxl-card

@kertuilves kertuilves force-pushed the kertu/feat/dashboard-category-accordion branch from efa005c to 0f8f7f8 Compare July 14, 2023 10:43
@kertuilves kertuilves mentioned this pull request Aug 1, 2023
Merged
@kertuilves kertuilves force-pushed the kertu/feat/dashboard-category-accordion branch 3 times, most recently from 5e1ae3e to 22609a3 Compare August 8, 2023 09:20
@kertuilves kertuilves force-pushed the kertu/feat/dashboard-category-accordion branch from 22609a3 to bb5e8ee Compare August 8, 2023 09:24
@pawelkmpt pawelkmpt merged commit b02e9c2 into feat/dashboard Aug 11, 2023
4 checks passed
@pawelkmpt pawelkmpt deleted the kertu/feat/dashboard-category-accordion branch January 9, 2024 12:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pawelkmpt pawelkmpt pawelkmpt approved these changes

@lkraav lkraav Awaiting requested review from lkraav

@anoblet anoblet Awaiting requested review from anoblet

Assignees
No one assigned
Labels
CXL 2.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@kertuilves @heshfekry @pawelkmpt @anoblet @phkel