Skip to content

CdkListbox file skeleton #19612

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

Merged
merged 20 commits into from
Jun 17, 2020
Merged

CdkListbox file skeleton #19612

merged 20 commits into from
Jun 17, 2020

Conversation

nielsr98
Copy link
Contributor

Created directory for listbox in cdk-experimental and created empty files that will eventually contain the listbox and listbox option directives.

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jun 11, 2020
@nielsr98 nielsr98 requested review from jelbourn and scriby June 11, 2020 19:39
"//src/cdk/portal",
"@npm//@angular/animations",
"@npm//@angular/core",
"@npm//rxjs",
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if this file has been built up from an existing prototype you already have, but generally I'd prefer us adding these dependencies on-demand. Otherwise we might end up with unused dependencies that slow-down CI and local development (same for the loaded, but unused rules above)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah okay that makes sense. Yeah I'll just remove it and then add them as needed.

@jelbourn
Copy link
Member

On the file names: we typically don't suffix file names with -directive, so I would just do listbox.ts and option.ts. I would also just add an empty export class CdkListbox { } (and option) along with a build rule for the package to confirm that everything compiles as expected

@nielsr98 nielsr98 requested a review from a team as a code owner June 12, 2020 04:11
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -127,6 +127,7 @@
/src/cdk-experimental/dialog/** @jelbourn @crisbeto
/src/cdk-experimental/popover-edit/** @kseamon @andrewseguin
/src/cdk-experimental/scrolling/** @mmalerba
/src/cdk-experimental/listbox/** @nielsr98
Copy link
Member

Choose a reason for hiding this comment

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

You can add me here also

exportAs: 'cdkListbox',
host: {
role: 'listbox',
tabindex: '0'
Copy link
Member

Choose a reason for hiding this comment

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

I would leave off the tabindex and deal with focus management in a follow-up PR focused just on dealing with focus/activedescendant

@jelbourn jelbourn added merge: fix commit message When the PR is merged, rewrites/fixups of the commit messages are needed lgtm action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release labels Jun 12, 2020
@andrewseguin
Copy link
Contributor

Can you rebase the change

@googlebot
Copy link

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla and removed cla: yes PR author has agreed to Google's Contributor License Agreement labels Jun 14, 2020
@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: yes PR author has agreed to Google's Contributor License Agreement and removed cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla labels Jun 15, 2020
@jelbourn
Copy link
Member

This is rebased and ready to go


const EXPORTED_DECLARATIONS = [CdkListbox, CdkOption];
@NgModule({
exports: EXPORTED_DECLARATIONS,
Copy link
Contributor

Choose a reason for hiding this comment

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

Excuse me but it looks like it's indented by 4, not 2 (here and listbox.ts).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, thanks.

@nielsr98 nielsr98 added target: minor This PR is targeted for the next minor release and removed target: patch This PR is targeted for the next patch release labels Jun 17, 2020
@mmalerba mmalerba merged commit 0428231 into angular:master Jun 17, 2020
devversion pushed a commit to devversion/material2 that referenced this pull request Jun 18, 2020
* build: created listbox directory and BUILD.bazel file.

* build: filled BUILD.bazel file.

* build: Added required files to listbox directory.

* build: added listbox option directive and renamed listbox directive files.

* fix: Removed currently unused dependencies from BUILD.bazel. Added listbox to cdk-experimental entry points.

* Removed BUILD.bazel for temp test

* fix: filled out BUILD.bazel and other required listbox files.

* fix: changed double quote to single quote and added listbox module to public-api.

* fix: semicolon.

* removed tabindex from listbox host and changed option directive name.

* build: Added required files to listbox directory.

* build: added listbox option directive and renamed listbox directive files.

* fix: filled out BUILD.bazel and other required listbox files.

* build: Added required files to listbox directory.

* build: added listbox option directive and renamed listbox directive files.

* build: Added required files to listbox directory.

* build: added listbox option directive and renamed listbox directive files.

* fix: reduced indent size from 4 to 2.

* fix: chore, removed empty files that appeared after rebase.

* fix: added back menu to config.bzl
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Jul 18, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement merge: fix commit message When the PR is merged, rewrites/fixups of the commit messages are needed target: minor This PR is targeted for the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants