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

Updating dialogTitle prop to be a string or element #234

Merged
merged 1 commit into from
Sep 20, 2020

Conversation

4-Eyes
Copy link
Contributor

@4-Eyes 4-Eyes commented Aug 11, 2020

Description

  • Adding the ability to customise the dialog title with JSX, rather than it just being a string.

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested

  • Adding to the documentation page for the DropzoneDialogBase an example that includes a custom dialog title with an 'x' button for closing the dialog.

Test Configuration:

  • Browser: Chrome Version 84.0.4147.105

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@panz3r panz3r self-requested a review August 24, 2020 09:18
@panz3r panz3r added this to the 3.5.0 milestone Aug 24, 2020
Copy link
Contributor

@panz3r panz3r left a comment

Choose a reason for hiding this comment

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

Hi @4-Eyes ,

Thanks for your contribution and sorry for the delayed answer.

I added some comments and I'd ask you to also update TS typings to match the changes.

.gitignore Outdated Show resolved Hide resolved
.size-snapshot.json Outdated Show resolved Hide resolved
dist/index.es.js Outdated Show resolved Hide resolved
dist/index.es.js.map Outdated Show resolved Hide resolved
dist/index.js Outdated Show resolved Hide resolved
dist/index.js.map Outdated Show resolved Hide resolved
@4-Eyes
Copy link
Contributor Author

4-Eyes commented Sep 11, 2020

Apologies for the slow response. I believe I've addressed all your comments.

@4-Eyes 4-Eyes requested a review from panz3r September 11, 2020 20:16
Copy link
Contributor

@panz3r panz3r left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@4-Eyes , thanks for your contribution, now it looks good but unfortunately GitHub says I cannot merge it because of conflicts, can you please rebase your changes (only the revenant ones) onto master and update this PR?
Thanks again

@4-Eyes 4-Eyes reopened this Sep 19, 2020
@4-Eyes
Copy link
Contributor Author

4-Eyes commented Sep 19, 2020

@panz3r the PR should be updated now.

Copy link
Contributor

@panz3r panz3r left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@panz3r panz3r merged commit e5d280e into Yuvaleros:master Sep 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants