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

Modal footer divider persists when buttons removed #5462

Closed
LyricL-Gitster opened this issue Mar 23, 2017 · 11 comments
Closed

Modal footer divider persists when buttons removed #5462

LyricL-Gitster opened this issue Mar 23, 2017 · 11 comments
Labels
🐛 Bug Ant Design Team had proved that this is a bug. help wanted The suggestion or request has been accepted, we need you to help us by sending a pull request.

Comments

@LyricL-Gitster
Copy link

LyricL-Gitster commented Mar 23, 2017

The Modal component shows the bottom divider when the footer prop is set to null.

Environment(required)

  • antd version 2.8.0:
  • macOS Sierra (version 10.12.3):
  • Chrome version 56.0.2924.87 (64-bit)

What did you do? Please provide steps to re-produce your problem.

  1. Render Modal component with the visible prop true and the footer prop as some non-null element.
  2. Update the footer prop to null
  3. BUG bottom divider continues to show even though footer content is null.

What do you expected?

I would expect the footer section and divider to be hidden as it is in this image.
footer_hidden

What happen?

It is not hidden.
divider_visible

@NE-SmallTown
Copy link
Contributor

NE-SmallTown commented Mar 24, 2017

Check this you will find that we just add your custom component to the footer container.

So if you want to implement your goal,you should make some change in the src code such as add a property in footer i.e.:

if(props.footer) {
   if(props.footer.isCustom) {
       footer = {props.footer};
   } else {
       footer = (
        <div className={`${prefixCls}-footer`} ref="footer">
          {props.footer}
        </div>
      );
   }
)

Of course you can do this via judging props.footer is null(remember update the doc too~),at now we use if(props.footer),so null and undefined will be treated as the same.Or you can just set style to overwrite the border to make less code and default css works fine.That's up to you.

Welecome pr~

@benjycui
Copy link
Contributor

It seems that we cannot set Modal[footer=null] to hide footer now.

But we can update this line of code to achieve your target:

footer={footer === undefined ? defaultFooter : footer}

@benjycui benjycui added 🐛 Bug Ant Design Team had proved that this is a bug. help wanted The suggestion or request has been accepted, we need you to help us by sending a pull request. labels Mar 24, 2017
@afc163
Copy link
Member

afc163 commented Mar 24, 2017

You can use footer={[]} now.

@benjycui
Copy link
Contributor

footer=[] will not work as @llhupp 's expectation:

image

@reid3290
Copy link

Is this actually a bug? I have used antd for a while in my project, but i never dive into the codes. Is this a good one for beginner to start with? Thx.

@afc163
Copy link
Member

afc163 commented Mar 24, 2017

@reid3290 Yes, I think it is a pretty clear one and the solution of @benjycui will works well. PR is always welcome.

@NE-SmallTown
Copy link
Contributor

NE-SmallTown commented Mar 24, 2017

IMO,maybe we can provide more fine-grained api like I say avove.Because there must be some people will consider that keep the bottom line is better for UX.So base on this,if you just want to achieve your target,set border be none in your css to overwrite the style is better.And if you want to pr,change the footer api to a object such as footer {object} : {element : node,showBorder: boolean......} is better and if you just pass a node it also works fine because we will give showBorder a default value.

@reid3290
Copy link

Fair point. But IMO, the top border of the footer is kind of a divider to seperate it from the content. If you don't need the footer, then you don't need the divider. I think this is more common. If some people really want the divider but not the footer, it makes more sense to add bottom border to the content div. Changing the footer api would make it unnecessarily tedious.

@NE-SmallTown
Copy link
Contributor

Yeah~That's up to you,just hope maybe it will inspired you.For me both of them are ok.

@LyricL-Gitster
Copy link
Author

Thanks, everyone! The library looks great and I look forward to trying out the fix. As a side note, we found a workaround in 2.8.0 where we remount the modal when we change the footer.

@lock
Copy link

lock bot commented May 1, 2018

This thread has been automatically locked because it has not had recent activity. Please open a new issue for related bugs and link to relevant comments in this thread.

@lock lock bot locked as resolved and limited conversation to collaborators May 1, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🐛 Bug Ant Design Team had proved that this is a bug. help wanted The suggestion or request has been accepted, we need you to help us by sending a pull request.
Projects
None yet
Development

No branches or pull requests

5 participants