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

Update SUIR to be compatible with SUI 2.3 #2550

Closed
4 of 5 tasks
brianespinosa opened this issue Feb 20, 2018 · 21 comments
Closed
4 of 5 tasks

Update SUIR to be compatible with SUI 2.3 #2550

brianespinosa opened this issue Feb 20, 2018 · 21 comments

Comments

@brianespinosa
Copy link
Member

brianespinosa commented Feb 20, 2018

Creating this as a container for referencing other issues that are likely to pop up with the release of SUI 2.3 which happened today. Feel free to edit this and add related issues below for tracking.

Notes about what is new in this release:
https://semantic-ui.com/introduction/new.html

Full CHANGELOG:
https://github.com/Semantic-Org/Semantic-UI/blob/master/RELEASE-NOTES.md

Known issues so far. Please edit this list to add or link to issues:

@levithomason
Copy link
Member

Oh, great idea. I think we just ship this as a new breaking change. We can then bundle all 2.3.x changes into one release. Thanks for starting the list!

@brianespinosa
Copy link
Member Author

Updated the list with a few more and also added a link to the full CHANGELOG. There may be one or two I missed. And with how we render popups via a Portal, I am not sure we have to worry about the update allowing you to trigger a popup from other blocks of markup.

@levithomason
Copy link
Member

I think we're ready to go here. Changing labels.

@brianespinosa
Copy link
Member Author

@davidhenley any issues tagged with help wanted are open for anyone to submit PRs to fix. You are welcome to do so. Any info related to this and all issues will be shown above in the issue history. Based on reading this history, it looks like there are no open PRs yet.

@Errolyn
Copy link

Errolyn commented Mar 14, 2018

Any chance your Modal documentation can be updated to reflect the modal placement issue?

@amoerie
Copy link

amoerie commented Mar 14, 2018

In any case, it seems wise to add a warning on the docs homepage that Semantic UI React is not officially compatible with Semantic UI 2.3 yet, and that using the 2.3 css in combination with the latest react version is at your own risk until this issue is resolved.

@brianespinosa
Copy link
Member Author

Valid point @Riddlerat, @amoerie. Since this issue was not picked up quickly for work, I have submitted a quick PR to update the README.md in #2644 linking here to alert people until we get a patch on this.

Fingers crossed, I am hoping to at minimum get the modal working with 2.3 over the weekend.

@layershifter
Copy link
Member

See #2657, I've made updates for Dimmer and Transition. We need updates for Modal and Icon before we can ship it.

@brianespinosa
Copy link
Member Author

brianespinosa commented Mar 19, 2018

@layershifter I have some time blocked out tomorrow afternoon to tackle Modal. Do you mind if I just add those commits into your PR branch? Nevermind. I see you merged your separate PRs into next already.

If I have the time I may also be able to tackle Icon. I think there were a LOT of additions in this new version of FontAwesome so that might take a while, but I have not looked into seeing if there is an easy to parse list in the CSS on SUI somewhere yet.

@layershifter
Copy link
Member

@brianespinosa I've merged PR for Icon to next, with generator, BTW 😸

@ghost
Copy link

ghost commented Mar 20, 2018

There is a message for Modals in 2.3.1: https://github.com/Semantic-Org/Semantic-UI/blob/master/RELEASE-NOTES.md.

A Special Message about Flex Modals There will be an update shortly to resolve issues related to flex modals when using multiple modals and detachable: false, in order to not hold up this release, we've decided to move forward without a fix.

A general solution will most likely require branching code for IE11 which will disable flex (as IE11 doesnt correctly implement the latest spec for absolute positioned flex containers).

@brianespinosa
Copy link
Member Author

brianespinosa commented Mar 21, 2018

In light of the release notes for 2.3.1 referenced above, it seems like support for either version of the modals is the long term strategy for SUI core styles. I need to dig in over there to find out for sure, but if this is the case, we'll probably have to go with my earliest proposal in this comment.

The question here is timing. This is actually a fairly significant change. I think some people will potentially have install bases of CSS that they will not be able to update/recompile soon. I am proposing what we do is add the ability to keep the existing functionality for modals for the time being if people want by adding a boolean flex prop to Modal. If flex is true, the above two changes would be enabled. If flex is false, then the old way would still be in place.

If indeed there will be the ability to go either way on this in the styles, I'll make sure the PR has the above option for our modals as well.

@brianespinosa
Copy link
Member Author

I haven't received a reply yet on my question here for @jlukic so I think for now I am just going to proceed without some sort of legacy/fallback prop. I'll try to keep a close eye on whatever happens with modals in the next version of SUI since it sounds like there are going to be some additional breaking changes incoming.

Starting a branch on this now in the office as I've blocked out some official work hours to get this working. PR should be up this afternoon.

@brianespinosa
Copy link
Member Author

To update on Modal positioning, see this comment from the linked issue in SUI:

Most likely this will be flexbox for all browsers except some kind of fallback for IE11 that uses javascript positioning. There might be a world where I can use flexbox without absolute positioning, but it would require modifying the DOM structure when using multiple modals, which would be bad for anyone who expects them to be adjacent.

@franckboudraa
Copy link

Quick fix for modal positioning, working here with SUI 2.3.1 & SUIR 0.79.1:

.visible.transition {
  display: unset !important;
}

.dimmed.dimmable > .ui.visible.dimmer, .ui.active.dimmer {
  display: -ms-flexbox;
  display: flex !important;
  opacity: 1;
}

@brianespinosa
Copy link
Member Author

@franckboudraa, and for anyone landing here, I would recommend against what you have proposed for .visible.transition. Those classes are used for lots of other things in the SUI css, not just modals.

A safer solution for anyone wanting to use the newest version of SUI styles and this incompatible version of SUIR would be safer doing the following:

.ui.dimmer {
  display: flex !important;
}

.ui.modal {
  margin-top: 0;
}

There are other problems you'll encounter using the latest styles, but those above changes should at least fix modals. The last fix for this is coming in PR #2689.

@reddishtg
Copy link

This may be part of what @brianespinosa was referring to regarding Icons, but since the shift to FontAwesome 5, an error is being thrown for "invalid" icon names. They are raising an error (it looks like) because SUI.js does not have the correct list of icon names for FA5. Thankfully, the icons still show correctly on the page.

@brianespinosa
Copy link
Member Author

brianespinosa commented Apr 30, 2018

@reddishtg those are only proptype errors. If you are not using another component broken by these changes and want to continue using a newer version of the CSS than we currently support until #2657 gets merged and deployed, you can just declare your icon names in theclassName prop instead of the name prop on your <Icon /> component.

@arnm
Copy link

arnm commented May 25, 2018

What versions of semantic-ui-react and semantic-ui-css are sufficiently compatible with each other?

It is very difficult for a newbie, like myself, to use this library when it is unclear which versions of libraries should be used together. Placing this information, if available, in the README would be a huge help.

@layershifter
Copy link
Member

semantic-ui-react currently compatible only with semantic-ui-css@2.2.

@Semantic-Org Semantic-Org locked as too heated and limited conversation to collaborators May 25, 2018
@brianespinosa
Copy link
Member Author

@arnm Please take a look at the VERY first thing at the top of the README.md

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

8 participants