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(cxl-ui): [cxl-marketing-nav] add descriptions #220

Merged
merged 2 commits into from
Sep 16, 2022
Merged

feat(cxl-ui): [cxl-marketing-nav] add descriptions #220

merged 2 commits into from
Sep 16, 2022

Conversation

saas786
Copy link

@saas786 saas786 commented Sep 6, 2022 •
edited
Loading

Closed old PRs:

and created fresh branch & PR by bringing the changes from above PRs.

@kylebrodeur
Copy link

Task linked: CU-2x7c7dv Aybolit - Nav Menu items description feature.

@github-actions
Copy link

github-actions bot commented Sep 6, 2022 •
edited
Loading

size-limit report 📦

Path Size
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 58.9 KB (+0.47% 🔺)

@saas786
Copy link
Author

saas786 commented Sep 6, 2022

@anoblet

Which menu should show description, I had a look at My Dashboard but it doesn't seem to show description.

Ref: https://deploy-preview-220--conversionxl-aybolit.netlify.app/?path=/story/cxl-ui-cxl-marketing-nav--cxl-marketing-nav

@saas786 saas786 added the enhancement New feature or request label Sep 6, 2022
@anoblet
Copy link
Collaborator

anoblet commented Sep 6, 2022

@saas786

It seems Vaadin got rid of contextMenu.render(), I updated it to contextMenu.requestContentUpdate(). You should now be able to see a description under My Dashboard.

This was referenced Sep 7, 2022
Closed
Closed
@saas786 saas786 force-pushed the anoblet/feat/cxl-marketing-nav-descriptions branch 5 times, most recently from afd0a9a to 7d20e64 Compare September 8, 2022 09:34
@anoblet anoblet force-pushed the anoblet/feat/cxl-marketing-nav-descriptions branch from 2efadb5 to b6caffb Compare September 9, 2022 14:06
@saas786 saas786 force-pushed the anoblet/feat/cxl-marketing-nav-descriptions branch 5 times, most recently from e846965 to 43a3907 Compare September 12, 2022 09:43
@saas786 saas786 force-pushed the anoblet/feat/cxl-marketing-nav-descriptions branch from 43a3907 to 395441b Compare September 12, 2022 09:46
@saas786 saas786 force-pushed the anoblet/feat/cxl-marketing-nav-descriptions branch 3 times, most recently from 7fe9e37 to 1a6bf59 Compare September 12, 2022 12:34
@saas786 saas786 marked this pull request as ready for review September 12, 2022 12:36
@saas786 saas786 force-pushed the anoblet/feat/cxl-marketing-nav-descriptions branch from a1ed2cf to a9f4b73 Compare September 12, 2022 12:52
lkraav
lkraav previously requested changes Sep 14, 2022
View reviewed changes
packages/storybook/cxl-ui/cxl-marketing-nav.data.json
{
"depth": 1,
"text": "Skip Trial Period",
"description": "Gain unlimited instant access to all CXL features.",
Copy link

Choose a reason for hiding this comment

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

Can y'all get some realistic descriptions from growth / product team, and add to this data file? Need better answers to "What exactly are they planning to do with this feature?"

It's difficult to make tech design decisions with just a single random lorem ipsum-like example.

Copy link

Choose a reason for hiding this comment

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

@saas786 @anoblet can we get it today? This is blocking two PRs:

and important feature @kylebrodeur is waiting for.

Copy link
Collaborator

@anoblet anoblet Sep 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I asked @saas786 , and he said this PR should be good to go. @pawelkmpt do you see any changes that you think need to be made?

Copy link

@pawelkmpt pawelkmpt Sep 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I do not, @lkraav has. I see you created #224 so maybe we can add it separately.

Copy link
Author

Choose a reason for hiding this comment

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

Yes I see #224 too.

I only added Trial related menu item and also synced live menu for My Account for testing puproses.

CXL Marketing Nav certainly should reflect institute menu items, Lorem Ipsum or Demo items doesn't help much.
Will allow us to test live menu items whenever we change something in CXL Marketing Nav or related elements.

Rather than discovering issues on live website.

@pawelkmpt pawelkmpt dismissed lkraav’s stale review September 16, 2022 09:36

It takes to long for Leho to re-check and we need to merge it.

@pawelkmpt pawelkmpt merged commit dca841b into master Sep 16, 2022
@saas786 saas786 deleted the anoblet/feat/cxl-marketing-nav-descriptions branch October 31, 2022 07:30
@saas786 saas786 restored the anoblet/feat/cxl-marketing-nav-descriptions branch October 31, 2022 07:30
@lkraav lkraav deleted the anoblet/feat/cxl-marketing-nav-descriptions branch December 7, 2022 11:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anoblet anoblet anoblet left review comments

@pawelkmpt pawelkmpt pawelkmpt approved these changes

@lkraav lkraav Awaiting requested review from lkraav

Assignees

@anoblet anoblet

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@saas786 @kylebrodeur @anoblet @lkraav @pawelkmpt