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

Add draft for File input #140

Merged
merged 1 commit into from
Feb 16, 2021
Merged

Add draft for File input #140

merged 1 commit into from
Feb 16, 2021

Conversation

mrmckeb
Copy link
Collaborator

@mrmckeb mrmckeb commented Jul 22, 2020

Relates to PR #92, issue #114.

@yoavweiss
Copy link

@mrmckeb could you join the WICG to appease the IPR bots?

@mrmckeb
Copy link
Collaborator Author

mrmckeb commented Jul 28, 2020

Hi @yoavweiss, thanks for the note. I did - after creating this PR - but it doesn't seem to have updated... maybe the bot can be re-run?

@yoavweiss
Copy link

Re-ran the bots and they seem to like you now :) Welcome!!

@mrmckeb
Copy link
Collaborator Author

mrmckeb commented Jul 28, 2020

The stars have aligned, thanks @yoavweiss!

Copy link
Member

@gregwhitworth gregwhitworth left a comment

Choose a reason for hiding this comment

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

Thanks for your time on this. I requested a few issues to be opened for group discussion.

Can you please add in an events section that shows which events are tied to which parts and their resulting impacts on the various other states and parts? Here is an example of this in <select> https://open-ui.org/components/select#events-1

research/src/pages/file/file.proposal.mdx Outdated Show resolved Hide resolved
research/src/pages/file/file.proposal.mdx Outdated Show resolved Hide resolved
research/src/pages/file/file.proposal.mdx Outdated Show resolved Hide resolved
research/src/pages/file/file.proposal.mdx Outdated Show resolved Hide resolved
research/src/pages/file/file.proposal.mdx Outdated Show resolved Hide resolved
research/src/pages/file/file.proposal.mdx Show resolved Hide resolved
research/src/pages/file/file.proposal.mdx Show resolved Hide resolved
research/src/pages/file/file.proposal.mdx Show resolved Hide resolved
research/src/pages/file/file.proposal.mdx Show resolved Hide resolved
research/src/pages/file/file.proposal.mdx Outdated Show resolved Hide resolved
@gregwhitworth
Copy link
Member

ping @mrmckeb any update on this?

Copy link
Member

@gregwhitworth gregwhitworth left a comment

Choose a reason for hiding this comment

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

I left some feedback on some minor tweeks and an additional issue or two for further discussion. Thanks for taking the time on this - it looks good.

research/src/pages/file/file.proposal.mdx Outdated Show resolved Hide resolved
research/src/pages/file/file.proposal.mdx Show resolved Hide resolved
research/src/pages/file/file.proposal.mdx Outdated Show resolved Hide resolved
research/src/pages/file/file.proposal.mdx Show resolved Hide resolved
research/src/components/file-anatomy.js Outdated Show resolved Hide resolved
@gregwhitworth
Copy link
Member

@mrmckeb gentle ping, any update?

Copy link
Member

@gregwhitworth gregwhitworth left a comment

Choose a reason for hiding this comment

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

Sorry for finding more changes, I apologize. I would like to land this. @una can you give a review of this as well so that @mrmckeb can possibly only do 1 more pass and we can land the PR.

research/src/components/file-anatomy.js Outdated Show resolved Hide resolved
research/src/pages/file/file.proposal.mdx Outdated Show resolved Hide resolved
research/src/pages/file/file.proposal.mdx Outdated Show resolved Hide resolved
research/src/pages/file/file.proposal.mdx Outdated Show resolved Hide resolved
research/src/components/file-anatomy.js Outdated Show resolved Hide resolved
@mrmckeb
Copy link
Collaborator Author

mrmckeb commented Dec 7, 2020

Thanks @gregwhitworth, I'll wait for @una's review and then make the final changes! Looking forward to getting this in :)

@mrmckeb mrmckeb requested a review from una as a code owner January 14, 2021 19:00
@mrmckeb mrmckeb mentioned this pull request Jan 14, 2021
Copy link
Member

@gregwhitworth gregwhitworth left a comment

Choose a reason for hiding this comment

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

There are some errors in the console that relate to the anatomy. Can you please ensure that those are addressed. We're almost there, only a few small changes. Thank you again for your work on this.

research/src/pages/file/file.proposal.mdx Outdated Show resolved Hide resolved
research/src/pages/file/file.research.mdx Show resolved Hide resolved
@mrmckeb
Copy link
Collaborator Author

mrmckeb commented Jan 19, 2021

Thanks @gregwhitworth, this is updated, and squashed.

@mrmckeb mrmckeb requested a review from gregwhitworth January 20, 2021 09:08
Copy link
Member

@gregwhitworth gregwhitworth left a comment

Choose a reason for hiding this comment

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

Please see the comment regarding the resolution of the tabbing behavior for #248. I don't want to block that edit on this PR but if you can submit a new PR once this one lands with that resolved change.

@mrmckeb
Copy link
Collaborator Author

mrmckeb commented Feb 1, 2021

Thanks @gregwhitworth - I can make that change as soon as this lands.

@gregwhitworth
Copy link
Member

@mrmckeb this is approved by me so we need to get one more to land it. I actually have a triage session with @una tomorrow so I'll make sure bring this to her attention.

@una
Copy link
Collaborator

una commented Feb 16, 2021

LGTM, nice breakdown

@una una merged commit 2025363 into openui:master Feb 16, 2021
@mrmckeb mrmckeb deleted the draft/file-input branch February 17, 2021 11:08
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.

4 participants