-
Notifications
You must be signed in to change notification settings - Fork 7
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
fix: toStyle, convert minWidth to min-width #149
Conversation
🦋 Changeset detectedLatest commit: f2a47bf The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Preview URLsGH Env: preview |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm.
you'll want to add a changeset entry though
Hopefully unblocking: #150 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved, but as Preston noted, this should have a changelog entry, and cause a version bump, otherwise we can't get this released (I think?):
Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me as well. I'm curious if we have any tests around this as well?
Yeah I think it might be this test: https://github.com/CrowdStrike/ember-headless-table/blob/main/test-app/tests/plugins/sticky-columns/rendering-rfc-883-test.gts Will take a look at updating it...looks like we test there for stickiness but not the |
await helpers.scrollRight(200); | ||
|
||
/** | ||
* Because we only scrolled the distance of one column for this test, | ||
* it doesn't make sense to check the other columns. | ||
*/ | ||
left = leftPositionOf('A'); | ||
await this.pauseTest(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a stray pause!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks a lot like an issue with ember-auto-import, where @cached
isn't defined.
See: embroider-build/ember-auto-import#504
Make sure you're using ember-auto-import@^2.6.2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you SO MUCH!! This fixes it...going to put up another PR for this.
418eb90
to
452bec9
Compare
452bec9
to
4b086d8
Compare
b470f5d
to
f2a47bf
Compare
Description
Some folks are having issues rendering the columns
minWidth
style because they style tag looks like:Instead of:
This hopefully fixes that?