Refactor Nav/Navbar/Dropdown's LinkTo components to not extend from Ember's LinkComponent #1455

simonihmig · 2021-03-14T23:31:43Z

According to RFC671 and deprecations introduced in latest Ember Canary.

Should fix the last remaining deprecation, see #1440.

addon/components/bs-link-to.js

simonihmig · 2021-03-14T23:44:24Z

addon/components/bs-link-to.js

+      return;
+    }
+
+    params = params.slice();


We haven't formally deprecated positional params for link params it seems. While this is now deprecated for Ember's original LinkComponent, we do not trigger this deprecation anymore. So we should deprecate this explicitly by ourselves I think. Should be a follow up PR.

Haven't we dropped support for curly-bracket component invocation in v4? If so, there shouldn't be a public API to provide positional params to a component. Or did I missed something?

Haven't we dropped support for curly-bracket component invocation in v4

We haven't declared so, no. We dropped arguments for HTML attributes, and more or less recommended to use angle brackets, but haven't explicitly stopped support for curly invocation. At the time there was no reason for this, and this is still not deprecated from Ember's side AFAIK. Except for this positional arguments issue there is no reason why curly invocation should bother us, as long as Ember supports it we can basically not care about it.

We can choose to drop support in a next major, or just say that the use of our custom link component drops support for positional arguments and all the other things (currentWhen) that accidentally became part of our implementation due to extending from LinkComponent, whether or not we consider this a public API or not. (it's a "gray area" I guess)

IIRC you were more on the side of accepting major versions more often, right? So should we actually do this refactoring as part of a v5, with just this edge case-y thing as the (potentially, but rather rare) breaking change?

simonihmig · 2021-03-14T23:56:29Z

The deprecation in Canary is gone, but something else related to FastBoot is failing now... 🤔

jelhan

Thanks a lot for picking this one up!

Was wondering why you decided to have a base class, which is than extended. Wouldn't implementation be easier to follow and require less code duplication, if <BsLinkTo> is invoked in templates instead of extending from it?

If we can avoid much of the complexity by dropping support for position params, currentWhen and activeClass arguments and may accept some differences in edge cases I would be fully onboard. I'm a little bit afraid of the maintenance burden if trying to exactly replicate current functionality.

addon/components/bs-link-to.js

jelhan · 2021-03-26T21:50:48Z

addon/components/bs-link-to.js

+      return;
+    }
+
+    params = params.slice();


Haven't we dropped support for curly-bracket component invocation in v4? If so, there shouldn't be a public API to provide positional params to a component. Or did I missed something?

jelhan · 2021-03-26T21:53:17Z

addon/components/bs-nav/link-to.js

 @uses Mixins.ComponentChild
 @public
 */
-@classNames(macroCondition(getOwnConfig().isBS4) ? 'nav-link' : '')
 export default class NavLinkTo extends LinkComponent.extend(ComponentChild) {}


Couldn't the component render <BsLinkTo> and passing the arguments forward instead of extending from it?

simonihmig · 2021-04-30T09:20:02Z

Was wondering why you decided to have a base class, which is than extended. Wouldn't implementation be easier to follow and require less code duplication, if is invoked in templates instead of extending from it?

I just tried that, which made me remember why: doesn't work for positional arguments. However if we want to drop them anyway - as part of a breaking change or not, see my other comment - then this makes sense!

simonihmig · 2021-04-30T15:59:28Z

I just tried that, which made me remember why: doesn't work for positional arguments. However if we want to drop them anyway - as part of a breaking change or not, see my other comment - then this makes sense!

I refactored this more, this is now yielding the BsLinkTo component directly, without extending it or even having to use a template-only wrapper. This still allows positional arguments, so no need for a semi-breaking change yet...

Will still look if I can remove the old cruft from the BsLinkTo component, i.e. just use the public router.isActive()...

simonihmig · 2021-05-13T16:38:27Z

Will still look if I can remove the old cruft from the BsLinkTo component, i.e. just use the public router.isActive()...

I have done this now as much as possible...

Our link component only uses public API of the router service, which is nice.
I removed that current-when and other stuff implicitly inherited from Ember's LinkComponent, as indeed we never indicated that was supported
However we did have tests for positional arguments! 😕 So removing those kinda feels like introducing a breaking change. I therefore left support, but will add a deprecation for those.
The link component is still an Ember.Component as it uses the ComponentChild mixin. We could get rid of the mixin itself, but still require the use of Ember.Component as we have to find the "parent" component (e.g. a nav item), for this annoying entangling of <a> having knowledge or the routing params and <li> needing to set the .active class. (Btw this is not the case anymore in BS5 it seems! 🎉)
This could be solved much easier if the navItem would yield the navLinkto component as it could then simply curry a parent=this argument. But unfortunately that's not our API, we yield the navLinkto from the nav itself 😔. I think we should change that (and deprecate the other way), right? Or accept it like it is today, and hope for people to switch to BS5 sooner than later? 🤔

…mber's LinkComponent According to RFC671 and deprecations introduced in latest Ember Canary.

…al public API

simonihmig · 2021-05-13T18:08:44Z

Finally CI is passing. @jelhan want to have another look?

jelhan

Looks very good. Thanks a lot for working on this.

addon/components/bs-link-to.js

jelhan · 2021-05-14T14:05:52Z

addon/components/bs-navbar/link-to.hbs

+  @replace={{@replace}}
+  @disabled={{@disabled}}
+  @current-when={{@current-when}}
+  @activeClass={{@activeClass}}


I don't think <BsLinkTo> supports @replace, @current-when and @activeClass. I guess a left-over from an earlier iteration.

Good catch, will fix!

Co-authored-by: Jeldrik Hanschke <jelhan@users.noreply.github.com>

simonihmig added the enhancement label Mar 14, 2021

simonihmig force-pushed the link-to branch from 5d63a86 to 9877e1c Compare March 14, 2021 23:35

simonihmig commented Mar 14, 2021

View reviewed changes

simonihmig requested a review from jelhan March 14, 2021 23:44

simonihmig changed the title ~~Refactor Nav/Navbar/Dropdown's LinkTo components to not extend from Ember's LinkComponent~~ WIP: Refactor Nav/Navbar/Dropdown's LinkTo components to not extend from Ember's LinkComponent Mar 14, 2021

jelhan reviewed Mar 26, 2021

View reviewed changes

This was referenced Apr 30, 2021

Failing builds in CI #1440

Closed

Fix deprecations #1477

Merged

simonihmig force-pushed the link-to branch from 9877e1c to 537e4a9 Compare April 30, 2021 15:56

simonihmig mentioned this pull request May 13, 2021

Support switch control in forms (BS4 only) #1495

Merged

simonihmig force-pushed the link-to branch from 6009a4f to cd5f5db Compare May 13, 2021 15:41

simonihmig added 8 commits May 13, 2021 19:05

Refactor Nav/Navbar/Dropdown's LinkTo components to not extend from E…

86f0c1b

…mber's LinkComponent According to RFC671 and deprecations introduced in latest Ember Canary.

Remove ignored deprecation

c78393a

Invoke <BsLinkTo> instead of extending it, refactor tests to use actu…

6e31148

…al public API

Refactor BsLinkTo to use only public router service APIs

fc39519

Fix test

cc9202e

Fixes for link-to query params

0433a9d

Fixes for link-to query params

c9347b6

Fixes after rebase

49e2edc

simonihmig force-pushed the link-to branch from 1353b4a to 49e2edc Compare May 13, 2021 17:11

simonihmig changed the title ~~WIP: Refactor Nav/Navbar/Dropdown's LinkTo components to not extend from Ember's LinkComponent~~ Refactor Nav/Navbar/Dropdown's LinkTo components to not extend from Ember's LinkComponent May 13, 2021

simonihmig requested a review from jelhan May 13, 2021 18:08

jelhan approved these changes May 14, 2021

View reviewed changes

simonihmig and others added 2 commits May 14, 2021 16:30

Add Ember version to autotracking comment

1b7f59f

Co-authored-by: Jeldrik Hanschke <jelhan@users.noreply.github.com>

Remove unused args

17537e0

simonihmig merged commit 4c11096 into master May 14, 2021

simonihmig deleted the link-to branch May 14, 2021 14:50

jelhan mentioned this pull request May 18, 2021

forward arguments in <BsNavbar::LinkTo> to <BsLinkTo> #1513

Merged

simonihmig mentioned this pull request May 19, 2021

Broken links markup in Navbars #1518

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Nav/Navbar/Dropdown's LinkTo components to not extend from Ember's LinkComponent #1455

Refactor Nav/Navbar/Dropdown's LinkTo components to not extend from Ember's LinkComponent #1455

simonihmig commented Mar 14, 2021 •

edited

simonihmig Mar 14, 2021

jelhan Mar 26, 2021

simonihmig Apr 30, 2021

simonihmig commented Mar 14, 2021

jelhan left a comment

jelhan Mar 26, 2021

jelhan Mar 26, 2021

simonihmig commented Apr 30, 2021

simonihmig commented Apr 30, 2021

simonihmig commented May 13, 2021

simonihmig commented May 13, 2021

jelhan left a comment

jelhan May 14, 2021

simonihmig May 14, 2021

Refactor Nav/Navbar/Dropdown's LinkTo components to not extend from Ember's LinkComponent #1455

Refactor Nav/Navbar/Dropdown's LinkTo components to not extend from Ember's LinkComponent #1455

Conversation

simonihmig commented Mar 14, 2021 • edited

simonihmig Mar 14, 2021

Choose a reason for hiding this comment

jelhan Mar 26, 2021

Choose a reason for hiding this comment

simonihmig Apr 30, 2021

Choose a reason for hiding this comment

simonihmig commented Mar 14, 2021

jelhan left a comment

Choose a reason for hiding this comment

jelhan Mar 26, 2021

Choose a reason for hiding this comment

jelhan Mar 26, 2021

Choose a reason for hiding this comment

simonihmig commented Apr 30, 2021

simonihmig commented Apr 30, 2021

simonihmig commented May 13, 2021

simonihmig commented May 13, 2021

jelhan left a comment

Choose a reason for hiding this comment

jelhan May 14, 2021

Choose a reason for hiding this comment

simonihmig May 14, 2021

Choose a reason for hiding this comment

simonihmig commented Mar 14, 2021 •

edited