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

Port @uppy/drop-target docs #59

Merged
merged 6 commits into from
Jan 24, 2023
Merged

Port @uppy/drop-target docs #59

merged 6 commits into from
Jan 24, 2023

Conversation

aduh95
Copy link
Member

@aduh95 aduh95 commented Jan 17, 2023

No description provided.

@aduh95 aduh95 mentioned this pull request Jan 17, 2023
7 tasks
@github-actions
Copy link
Contributor

github-actions bot commented Jan 17, 2023

PR Preview Action v1.2.0
Preview removed because the pull request was closed.
2023-01-24 19:51 UTC

Copy link
Member

@Murderlon Murderlon left a comment

Choose a reason for hiding this comment

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

Nice, thanks.

Some changes still needed:

  • It's missing the "When should I use it? section
  • Code sample is not under first heading but under "Use"
  • "Use" doesn't have a "CSS" section, instead if shows the plugin in context of other plugins with highlighting and showLineNumbers. Checkout some of the "Sources" plugins for examples.
  • "Options" doesn't show a code overview of the options. We have Generate table of contents under "Options" heading for all docs #45 for that.
  • Options do not show default values in the heading
  • The tip admonition for examples doesn't exist. The codesandbox link is also incorrect

Comment on lines +20 to +21
[Try out the live example](/examples) or take it for a spin in
[CodeSandbox](https://codesandbox.io/s/uppy-drag-drop-gyewzx?file=/src/index.js).
Copy link
Member

Choose a reason for hiding this comment

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

/examples doesn't exit. We should also create a codesandbox for this plugin and link it here.

Copy link
Member Author

Choose a reason for hiding this comment

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

We plan to have something at /examples don't we?

{ to: '/examples', label: 'Examples', position: 'left' },

Copy link
Member

Choose a reason for hiding this comment

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

No as discussed we will go live without it and potentially never have it as we emphasize our existing examples repo and codesandboxes. So at least for now we need to remove it.

Copy link
Member Author

Choose a reason for hiding this comment

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

We should also create a codesandbox for this plugin and link it here.

I don't know what's needed for this. We should probably document some guidelines around it.

No as discussed we will go live without it and potentially never have it as we emphasize our existing examples repo and codesandboxes.

There are 12 other pages linking to /examples, plus the main nav menu cited above. I don't think it makes sense to remove it from this PR. If we plan to remove them before going live, it can be done in a follow-up PR and doesn't have to block this one.

docs/user-interfaces/elements/drop-target.mdx Outdated Show resolved Hide resolved
docs/user-interfaces/elements/drop-target.mdx Outdated Show resolved Hide resolved
Comment on lines +94 to +97
#### `target`

DOM element, CSS selector, or plugin to place the drag and drop area into
(`string`, `Element`, `Function`, or `UIPlugin`, default: `null`).
Copy link
Member

Choose a reason for hiding this comment

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

I think we always put id and target on top of the list

Copy link
Member Author

Choose a reason for hiding this comment

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

Shouldn't we list the option in alphabetical order instead? That seems easier to enforce.

Copy link
Member

@Murderlon Murderlon Jan 19, 2023

Choose a reason for hiding this comment

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

It's a bit all over the place currently. I can make sorting part of #45 (automate it with remark). So indeed doesn't matter that much for now

Co-authored-by: Merlijn Vos <merlijn@soverin.net>

### Options

#### `onDragLeave(event)`
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to leave the function name with arguments, so URLs won’t break if we change the arguments? https://transloadit.slack.com/archives/C0FMW9PSB/p1673972430908959

@aduh95 aduh95 merged commit c7913e8 into main Jan 24, 2023
@aduh95 aduh95 deleted the drop-target branch January 24, 2023 19:49
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.

None yet

3 participants