Remove Application wrapper div #280
Conversation
cibernox
force-pushed
the
remove-wrapper-divs
branch
from
a2d0224
to
1afce81
Compare
|
|
||
| There is a possibility that removing the wrapper can break styles for some apps, | ||
| but since adding the wrapper back is just of editing the `application.hbs` template, | ||
| that is probably a despicable drawback. |
There was a problem hiding this comment.
It's despicable! (Daffy duck voice)
debatable?
There was a problem hiding this comment.
I was using it as a synonym of negligible.
Fixing small grammatical glitches
|
Yess! The current behavior gave me headaches in various styling scenarios. |
| that is probably a despicable drawback. | ||
|
|
||
| There is also a non-zero chance that some testing addon is relying on the `#ember-testing > .ember-view` | ||
| html hierarchy for some reason, and those addons would have to be updated. |
There was a problem hiding this comment.
FWIW qunit-dom supports both cases already via fallback if no ember-view child is found
|
The RFC says "This addon will be opt-in". Does that mean that there will be an ember-cli addon that will toggle this? I think that would be bad, better to just make it an ember setting/flag. Then make it standard (and remove setting/flag) in the next major release [breaking change]. |
|
@sandstrom The addon will only enable a "secret" flag in Ember. It will, as some point, be enabled by default and another addon will exist to disable it. I don't strongly oppose to just make it a flag passed to |
|
Got it! Although I disagree on this (addon vs setting) I'm still overall in favor of this RFC! 🏅 But sometimes I think ember is making addons out of stuff that should be settings. Some examples from ember-cli:
|
|
This is really due to some of the philosophy behind ember, the idea of having zero configuration and strong conventions around opinions. This way the default way of doing things is presented that can change with time in a manner that eases migration. When there is a setting and a default value, people are conditioned to consider the setting as something they can configure for their application. They tend to look at all the settings and feel free to customize. They look at defaults as "suggestions". We want them to instead consider the defaults as "conventions", where if you change them, you are making the decision to "stray from the herd". Another reason is "semver lawyering": if a default of a setting in the core changes, it is a violation of semver. But if an addon is added or removed in a blueprint for a new application, it isn't, "because that is not part of the core". Yet another reason is that any code for the non-default setting can be (although in this case it won't be) removed from the core and put in the addon if appropriate api hooks in the core are provided. |
|
I have implemented and landed this feature in master (behind a flag), so you can easily test the impact of this on canary now: https://ember-twiddle.com/04ea71656db0c3286d329f81c7eab1a4?numColumns=2&openFiles=twiddle.json%2Cstyles.app.css (updated link) |
|
not that i’m opposed to these changes, but why are we not doing to standard “final comment period” on this before adding it to master? Maybe i’m miss understanding the purpose of the FCP tag though. |
|
@webark I should have been more clear. It has landed in master behind a flag. The feature system is designed for this experimental implementation in mind – the code will be stripped in beta and release builds, and even on canary requires opting in (which is the whole purpose of the canary channel). For this particular change, it's something we (the core team and the community) has wanted for a long time, and the main questions are really around how to roll it out gracefully so it doesn't cause problems in the ecosystem. Having a working implementation allows us to answer those questions much more easily. This change is also very small and self-contained. Since all the code is already flagged, if we didn't end up wanting this change it would be easy to back it out. (The test changes are a nice refactor regardless.) |
|
And to be extra clear, we are still going to explore the options here and eventually going to do a FCP. The feature cannot be enabled before we do that and merging the RFC. |
|
Ahhh.. ok cool. Makes sense. Thanks for that clarification @chancancode ! |
|
👍 The prototypes are hopefully going to make it a lot easy to access the merits and impact of this change. (For example, @kategengler found a potential problem with the template-only components RFC yesterday by testing the interaction between that feature and the Ember Inspector.) |
|
This RFC was nominated and approved for Final Comment Period (FCP) yesterday, since most comments have already been replied to. The FCP with last for a week. If there are any substantial new arguments that are brought up during this time, the FCP will be restarted (or aborted). If you haven't reviewed the proposal yet, now would be a great time to do so. If I may, here is a summary of the main objections:
Meanwhile, since the feature have landed in canary behind a flag, I encourage you to give it a spin and get a feel for it, either using this Ember Twiddle or by using a canary build with the |
|
I think this change will be great. I fought with that div in styling before. What's the best/most likely way that someone would be alerted about this change once it's automatic, sometime in the future? If I upgraded my app and it suddenly looked different, it would take me a while to look at the top level of the DOM for a missing container. Most other version upgrades would throw a warning but this is styling, so it's sneaky. |
|
@jenweber the plan is this will be opt-in (by installing the addon) for the timeframe of 3.0. Towards the end of 3.0 we can issue a deprecation warning if you still don’t have it installed yet. |
|
|
||
| This RFC proposes to add a global flag to remove those wrapper elements, | ||
| effectively making the `application.hbs` template have "Outer HTML" semantics, which aligns | ||
| well with [the changes recently proposed](#278) for template-only components, |
There was a problem hiding this comment.
The link to #278 is broken here (lmk if it was left intentionally I'm gonna remove this comment)
|
This will help Ember apps become more accessible by default. When certain html5 elements are the direct descendant of the body element, an additional ARIA role is not required. While ember-a11y-landmarks provides a solution for current applications, I am encouraged to see a RFC that supports better native accessibility. |
|
Thanks everyone for your participation! Since no major issues were found during the FCP, we will now close and merge this proposal. |
These features are enabled by turning them into a runtime flag. These flags are intended to be set by @emberjs/ember-optional-features. In the future, these runtime flags might be removed in favor of build-time flags once the infrastructure is in place.
These features are enabled by turning them into a runtime flag. These flags are intended to be set by @ember/optional-features. In the future, these runtime flags might be removed in favor of build-time flags once the infrastructure is in place.
[FEATURE ...] Enable emberjs/rfcs#278, emberjs/rfcs#280
Wrapping the app is now useless since this RFC from Ember, for more explanation : emberjs/rfcs#280
Rendered