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(cxl-ui): center instructor image #406

Merged
merged 1 commit into from
Apr 24, 2024
Merged

Conversation

anoblet
Copy link
Collaborator

@anoblet anoblet commented Apr 17, 2024

Copy link

github-actions bot commented Apr 17, 2024

size-limit report 📦

Path Size
packages/cxl-ui/pkg/dist-web/cxl-ui.js 74.41 KB (+0.02% 🔺)
packages/cxl-ui/pkg/dist-web/cxl-ui-jwplayer.js 11.89 KB (0%)
packages/cxl-ui/pkg/dist-web/cxl-ui-playbooks.js 30.98 KB (0%)
packages/cxl-ui/pkg/dist-web/vendor.js 140.5 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 258.93 KB (+0.01% 🔺)

@@ -132,7 +132,7 @@
@media #{mq.$large} {
position: absolute;
bottom: calc(-1 * var(--lumo-space-xl));
right: calc(2 * var(--lumo-space-xl));
right: calc(27.5% - 200px);
Copy link

Choose a reason for hiding this comment

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

This is still a lot of magic numbers 🤔 is there no way to have some canonical generic numberless positioning formula here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm having trouble finding a formula that can calculate centering the image(400px) two thirds to the right of the container. The container is absolutely positioned which makes it difficult to just use relative units. I'm trying now in inspector, and using left: calc(50% + 100px) instead of right, looks a bit better. 100px being 1/4 of the width of the image.

Choose a reason for hiding this comment

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

I like left: calc(50% + 100px) more. Easier to understand.

Looks good on @media #{mq.$x-large}
but doesn't look perfect @media #{mq.$large} (1024px) - too close to the right edge.

Screenshot 2024-04-17 at 18 50 51 Screenshot 2024-04-17 at 18 51 00

@@ -132,7 +132,7 @@
@media #{mq.$large} {
position: absolute;
bottom: calc(-1 * var(--lumo-space-xl));
right: calc(2 * var(--lumo-space-xl));
right: calc(27.5% - 200px);

Choose a reason for hiding this comment

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

I like left: calc(50% + 100px) more. Easier to understand.

Looks good on @media #{mq.$x-large}
but doesn't look perfect @media #{mq.$large} (1024px) - too close to the right edge.

Screenshot 2024-04-17 at 18 50 51 Screenshot 2024-04-17 at 18 51 00

@pawelkmpt
Copy link

@anoblet ping

@anoblet
Copy link
Collaborator Author

anoblet commented Apr 23, 2024

Although it may seem like more of a magic number, right: calc(27.5% - 200px); displays better across all sizes. The rational behind it was the middle ground between 1/4, and 1/3 minus half of the image width.

@lkraav
Copy link

lkraav commented Apr 23, 2024

Although it may seem like more of a magic number, right: calc(27.5% - 200px); displays better across all sizes. The rational behind it was the middle ground between 1/4, and 1/3 minus half of the image width.

What about the option of JS inline style calculations based on actual image size?

@pawelkmpt
Copy link

Although it may seem like more of a magic number, right: calc(27.5% - 200px); displays better across all sizes. The rational behind it was the middle ground between 1/4, and 1/3 minus half of the image width.

What about the option of JS inline style calculations based on actual image size?

It'd be perfect but this is suppose to be a quick task. We don't want to spend on it more than we already did. Proposed solution is going live.

@pawelkmpt pawelkmpt merged commit 8fe6fa5 into master Apr 24, 2024
5 checks passed
@lkraav
Copy link

lkraav commented Apr 24, 2024

It'd be perfect but this is suppose to be a quick task. We don't want to spend on it more than we already did. Proposed solution is going live.

What exactly was spent here? Majority of the time it's been sitting waiting for review. This «giving up» style is a sure way to build into tech debt.

@pawelkmpt pawelkmpt deleted the anoblet/feat/featured branch April 25, 2024 03:50
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.

3 participants