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

Header no longer has rounded corners (regression) #308

Closed
romaninsh opened this issue Dec 17, 2018 · 10 comments
Closed

Header no longer has rounded corners (regression) #308

romaninsh opened this issue Dec 17, 2018 · 10 comments
Assignees
Labels
lang/css Anything involving CSS type/bug Any issue which is a bug or PR which fixes a bug

Comments

@romaninsh
Copy link

romaninsh commented Dec 17, 2018

Bug Report

After switching our project from Semantic to Fomantic, we noticed that dialog top edges are no longer rounded. After some digging we noticed that "header" had rounded corners before, but it is no longer the case:

/*******************************
             Modal
*******************************/

.ui.modal > :first-child:not(.icon),
.ui.modal > .icon:first-child + * {
  border-top-left-radius: 0.28571429rem;
  border-top-right-radius: 0.28571429rem;
}

Image before:
image

And after:
image

This affects all the dialogs.

Steps to reproduce

  1. TBA

Testcase

http://ui.agiletoolkit.org/demos/modal2.php

Version

2.6.4

@romaninsh
Copy link
Author

romaninsh commented Dec 17, 2018

Looks like it's caused by "ui inverted dimmer" which is now inserted as a first div inside a modal, and therefore breaks selector:

image

https://github.com/fomantic/Fomantic-UI/blob/master/src/definitions/modules/modal.less#L43

Perhaps .ui.modal > :first-child:not(.icon,.dimmer) could fix it?

@romaninsh
Copy link
Author

romaninsh commented Dec 17, 2018

Possibly that's a bug of atkjs, created related issue there, although strange that it appeared after the switch.

@y0hami
Copy link
Member

y0hami commented Dec 17, 2018

@romaninsh I reckon it is https://fomantic-ui.com/modules/modal.html#/examples all those examples seem fine

@lubber-de
Copy link
Member

Please adjust your (backend/atk)-code to not insert an additional manual dimmer div when working with modals.
Creating the Dimmer-tag for modals is handled by the module itself and is not positioned in the dom as shown.

@romaninsh
Copy link
Author

There is no example with a closing "X" and dimmer.

@romaninsh
Copy link
Author

romaninsh commented Dec 17, 2018

@lubber-de here is the modal creation code: https://github.com/atk4/ui/blob/develop/js/src/plugins/createModal.js

I don't see any tampering with the DOM.

@lubber-de
Copy link
Member

lubber-de commented Dec 17, 2018

There is no example with a closing "X" and dimmer.

😕 A modal always creates an internal dimmer and an example having a close icon can be found here https://fomantic-ui.com/modules/modal.html#full-screen

The created dom has the close icon as its first div child and the dimmer itself is surrounded

image

@lubber-de
Copy link
Member

I found it. it's used in the innerDimmer method of modal.js which creates the dimmer div you mentioned when allowMultiple is set to true.Please try if setting this to false basically solves the issue.
In the meantime i'll prepare a fiddle to reproduce this

@lubber-de lubber-de self-assigned this Dec 17, 2018
@lubber-de lubber-de added type/bug Any issue which is a bug or PR which fixes a bug lang/css Anything involving CSS labels Dec 17, 2018
@lubber-de
Copy link
Member

@romaninsh It's fixed in #310 and supposed to be part of the next release.
See http://jsfiddle.net/co3gaj8q/
Sorry for the misunderstanding 🙂

y0hami pushed a commit that referenced this issue Dec 17, 2018
When a modal had `allowMultiple:true` set, the top border radius was not set anymore.
This regression happens since an additional dimmer was introduced in #119 to prevent the ability to interact with modals that are not the focused one.
This PR now takes care of a possible existing extra dimmer and shows top border radius correctly again

Closes #308
@y0hami y0hami added the tag/next-release/nightly Any issue which has a corresponding PR which has been merged and is available in the nightly build label Dec 17, 2018
@romaninsh
Copy link
Author

@lubber-de no worries. My investigation was also missleading. Thanks for addressing it so promptly.

y0hami pushed a commit that referenced this issue Dec 18, 2018
When a modal had `allowMultiple:true` set, the top border radius was not set anymore.
This regression happens since an additional dimmer was introduced in #119 to prevent the ability to interact with modals that are not the focused one.
This PR now takes care of a possible existing extra dimmer and shows top border radius correctly again

Closes #308
This was referenced Dec 21, 2018
@y0hami y0hami closed this as completed in fb06e2f Dec 21, 2018
@lubber-de lubber-de removed the tag/next-release/nightly Any issue which has a corresponding PR which has been merged and is available in the nightly build label Dec 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lang/css Anything involving CSS type/bug Any issue which is a bug or PR which fixes a bug
Projects
None yet
Development

No branches or pull requests

3 participants