-
Notifications
You must be signed in to change notification settings - Fork 6.7k
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: move a11y, bidi, platform, rxjs, and portal to cdk #5386
Conversation
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.
I don't think that just copying the testing utilities is a good idea. We should do it correct from the beginning. Meaning that testing will be just used by the Material package instead of having it duplicated.
Edit: It's fine doing this in the future as a follow-up PR.
src/cdk/public_api.ts
Outdated
export * from './platform/index'; | ||
export * from './portal/index'; | ||
export * from './rxjs/index'; | ||
export * from './keyboard/keycodes'; |
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.
For the CDK I wanted the convention to always have an index.ts
file in the given piece of the CDK.
The index file of the "CDK piece" then always specifies what should be exported.
For the keycodes it might not be necessary, but for the sake of consistency I think it would make sense to do that as well (otherwise it will end up as in core.ts)
e6df5b0
to
752d653
Compare
@@ -0,0 +1,30 @@ | |||
# LiveAnnouncer | |||
`LiveAnnouncer` is a service, which announces messages to several screenreaders. |
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.
s/service/injectable? :p
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.
These will all get revised when we write overview files for stuff in the cdk like the rest of the library.
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.
Skipping the ripple test on iOS seems fine. Should reduce CI flakiness a lot.
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
import {NgModule} from '@angular/core'; | ||
import {DOCUMENT} from '@angular/platform-browser'; | ||
import {Dir} from './dir'; | ||
import {DIR_DOCUMENT, Directionality, DIRECTIONALITY_PROVIDER} from './directionality'; |
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.
Also export DIRECTIONALITY_PROVIDER_FACTORY
? It's used in core/
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.
Added
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.
Please fix the test?
Since we have line 14 we can remove line 12
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.
Yep, I clearly was not thinking. Fixed.
* Deletes the duplicate testing utilities because angular#5386 just copied them to the CDK but didn't delete the old files. * Introduces a secondary entry point (only inside of development right now) that allows us to use the testing utilities from the Material package tests
* Deletes the duplicate testing utilities because angular#5386 just copied them to the CDK but didn't delete the old files. * Introduces a secondary entry point (only inside of development right now) that allows us to use the testing utilities from the Material package tests
* Deletes the duplicate testing utilities because #5386 just copied them to the CDK but didn't delete the old files. * Introduces a secondary entry point (only inside of development right now) that allows us to use the testing utilities from the Material package tests
* Deletes the duplicate testing utilities because #5386 just copied them to the CDK but didn't delete the old files. * Introduces a secondary entry point (only inside of development right now) that allows us to use the testing utilities from the Material package tests
* feat: move a11y, bidi, platform, and portal to cdk * lint * Fix weird rxjs operator typing issue * Workaround for ngc issue * debug ie11 * debug ios * Add directionaltiy reexport
* Deletes the duplicate testing utilities because angular#5386 just copied them to the CDK but didn't delete the old files. * Introduces a secondary entry point (only inside of development right now) that allows us to use the testing utilities from the Material package tests
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
I'm purposefully not updating imports inside
lib/
to point to@angular/cdk
since we can do that later. This makes it easier to move other stuff to the cdk (since you don't have to then change the import again).I also screwed up and didn't make these "moves" in git.- if someone has a way to do that retroactively that would be nice.