Skip to content

feat(cxl-ui): add ability for main content to be scrolled from the ou…#15

Closed
anoblet wants to merge 7 commits intomasterfrom
external-scroll
Closed

feat(cxl-ui): add ability for main content to be scrolled from the ou…#15
anoblet wants to merge 7 commits intomasterfrom
external-scroll

Conversation

@anoblet
Copy link
Copy Markdown
Owner

@anoblet anoblet commented May 20, 2021

…tside

@lkraav
Copy link
Copy Markdown
Collaborator

lkraav commented Oct 13, 2021

Another customer request https://app.intercom.com/a/apps/wpyu0avy/inbox/inbox/all/conversations/41839800227390

@anoblet any thoughts on whether there's a more optimal solution of some kind?

@anoblet
Copy link
Copy Markdown
Owner Author

anoblet commented Oct 13, 2021

@leho I don't have access to the document :)

@lkraav
Copy link
Copy Markdown
Collaborator

lkraav commented Oct 14, 2021

I don't have access to the document :)

Intercom ticket isn't important, it just repeats the problem description of people not figuring out to scroll content pane.

Maybe there's a simpler solution of some kind than forwarding scroll events..

a) animate / shake the screen if scrolled outside, or something. They need a signal to go scroll in content pane.
b) re-architect CSS - make document scrollable, with sticky sidebar (seems a bit like a bunch of risky work?)
c) ...

@anoblet
Copy link
Copy Markdown
Owner Author

anoblet commented Oct 14, 2021

Maybe some sort of notification similar to Google Maps where they remind the user to use Ctrl + Scroll to zoom in?

I'll do some research :)

@anoblet
Copy link
Copy Markdown
Owner Author

anoblet commented Oct 25, 2021

I tried making CSS adjustments to see if we could accomplish what we needed, but ran into difficulties with the overlap of different layout structures.

We may want to consider breaking out each layout type into separate components.

https://www.loom.com/share/31d8a50fb48b4ce8a084ff1b384c688a

I'll start testing the Google Maps feature now.

@anoblet
Copy link
Copy Markdown
Owner Author

anoblet commented Oct 25, 2021

Here's the notification/highlight version:

https://www.loom.com/share/35e690e565944faeb71e5c2e5b02918b

Comment thread packages/cxl-ui/src/components/cxl-app-layout.js Outdated
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Oct 25, 2021

size-limit report 📦

Path Size
packages/cxl-ui/pkg/dist-web/index.bundled.js 101.17 KB (+1.24% 🔺)

@anoblet
Copy link
Copy Markdown
Owner Author

anoblet commented Oct 26, 2021

We might be able to use this to normalize the scrolling:

https://github.com/basilfx/normalize-wheel

@anoblet
Copy link
Copy Markdown
Owner Author

anoblet commented Oct 28, 2021

@leho I had to fork normalize-wheel and convert it to ESM because Pika wasn't building. It seems to work fairly well though.

Comment thread packages/cxl-ui/src/components/cxl-app-layout.js Outdated
Comment thread packages/cxl-ui/src/components/cxl-app-layout.js
"lit-element": "^2.2.1",
"lit-html": "^2.0.0-rc.3"
"lit-html": "^2.0.0-rc.3",
"normalize-wheel": "https://github.com/anoblet/normalize-wheel#esm"
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

If we have to fork this anyway, perhaps we can also drop some of the general compatibility check code from it? If not, that's fine, too, but worth a look.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

I'm not sure how much I can trim away without there being regressions in certain browsers but I can take a look.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is there a PR we can shoot at normalize-wheel upstream way?

I'm going to publish our current ESM build on our custom registry, but ideally we'd at one point get back to using global NPM for this.

Copy link
Copy Markdown
Owner Author

@anoblet anoblet Nov 12, 2021

Choose a reason for hiding this comment

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

I tried to structure it as a PR in the first place. I was unsure what the best practice was for offering CJS/ESM in the same package unbundled. A separate directory under src and leverage the browser field in package.json?

I ended up just installing from a branch to save time. I'll see if I can figure out how to do this though.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

I created a PR here: basilfx/normalize-wheel#3

In the meantime I believe we could rely on this: https://www.npmjs.com/package/normalize-wheel-es

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

https://www.npmjs.com/package/normalize-wheel-es

Looks like they did the same thing, and never bothered to PR.

Let's see what happens in basilfx/normalize-wheel#3 before we make a decision.

Ideally that PR would be a bit less noisy, or code-unified.. but I'm not so well-versed to understand how to pull it off.

@lkraav
Copy link
Copy Markdown
Collaborator

lkraav commented Nov 24, 2021

This was deployed live with conversionxl@8fd661f

@lkraav lkraav closed this Nov 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants