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.

Sign up for GitHub

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

Jump to bottom

feat: allow adding classes to both modal body (removing extraneous div) and backdrop #845

Merged
merged 4 commits into from
Jan 17, 2020
Merged

feat: allow adding classes to both modal body (removing extraneous div) and backdrop #845

merged 4 commits into from
Jan 17, 2020

Conversation

jbadan
Copy link
Contributor

@jbadan jbadan commented Jan 6, 2020

Description

Copy of #824, removing commit 01a4c6c that causes publishing issues.

fixes #800

@jbadan jbadan requested a review from a team January 6, 2020 16:48
@netlify
Copy link

netlify bot commented Jan 6, 2020 •
edited
Loading

Deploy preview for fundamental-react ready!

Built with commit 6259515

https://deploy-preview-845--fundamental-react.netlify.com

skvale
skvale approved these changes Jan 10, 2020
View reviewed changes
Copy link
Contributor

@skvale skvale left a comment

Choose a reason for hiding this comment

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

Looks good to me ⛵️

greg-a-smith
greg-a-smith suggested changes Jan 16, 2020
View reviewed changes
src/Modal/Modal.js Outdated
);

const modalClasses = classnames(
'modal-demo-bg',
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this class was here before, but should it be? This seems like something used as a selector for unit tests and there is an entry in customstyles.css from fundamental-styles. It's fine to use some custom styles for a playground or such, but it shouldn't be part of the base component's classes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! I'll remove it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@greg-a-smith turns out that entire div was just for the styles docs site. I'm removing it here and going to make a pr there to make that more obvious.

@jbadan jbadan changed the title fix: allow adding classes to both modal body and backdrop feat: allow adding classes to both modal body (removing extraneous div) and backdrop Jan 17, 2020
greg-a-smith
greg-a-smith approved these changes Jan 17, 2020
View reviewed changes
Copy link
Contributor

@greg-a-smith greg-a-smith left a comment

Choose a reason for hiding this comment

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

Looks good. 🚢

@greg-a-smith
Copy link
Contributor

@jbadan Although this is a feature, should the prefix be fix: since we are still on version 0?

@jbadan
Copy link
Contributor Author

jbadan commented Jan 17, 2020

I did feat because I think this technically a breaking change since I removed a div? What do you think?

@greg-a-smith
Copy link
Contributor

@jbadan Yeah, fair enough. I forgot about that. 👍

@jbadan jbadan merged commit f480cbc into master Jan 17, 2020
@jbadan jbadan deleted the fix/modal-classes branch January 17, 2020 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@greg-a-smith greg-a-smith greg-a-smith approved these changes

@skvale skvale skvale approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Modal does not honor provided CSS class in "className" property
3 participants
@jbadan @greg-a-smith @skvale