github-starred/actual
2
0
Fork 0
mirror of https://github.com/actualbudget/actual.git synced 2026-03-09 11:42:54 -05:00
Code Issues 250 Packages Projects Releases 53 Wiki Activity

[Tech Debt] ButtonLink, LinkButton, ExternalLink, AnchorLink #530

New Issue
Closed
opened 2026-02-28 19:07:49 -06:00 by GiteaMirror · 7 comments
GiteaMirror commented 2026-02-28 19:07:49 -06:00
Owner
Copy Link

Originally created by @MatissJanis on GitHub (Jul 29, 2023).

We have many components that act as fundamentally the same thing: a link.

  • ButtonLink - button that uses react-router for internal app navigation
  • LinkButton - a button that is styled in a specific way; this does not actually perform any navigation
  • ExternalLink - <a href> element for links to outside of the app (open in new tab)
  • AnchorLink - <a href> element for links inside the app (open in same window)

The goal of this ticket is to unify them. Before starting: please post the proposed interface in the comments.

Originally created by @MatissJanis on GitHub (Jul 29, 2023). We have many components that act as fundamentally the same thing: a link. - ButtonLink - button that uses react-router for internal app navigation - LinkButton - a button that is styled in a specific way; this does not actually perform any navigation - ExternalLink - `<a href>` element for links to outside of the app (open in new tab) - AnchorLink - `<a href>` element for links inside the app (open in same window) The goal of this ticket is to unify them. Before starting: please post the proposed interface in the comments.
GiteaMirror added the good first issuetech debt labels 2026-02-28 19:07:49 -06:00
GiteaMirror commented 2026-02-28 19:07:50 -06:00
Author
Owner
Copy Link

@MatissJanis commented on GitHub (Jul 29, 2023):

My recommendation:

Unify ButtonLink, ExternalLink and AnchorLink to a single component. And move LinkButton inside button with a new type (Button supports various styles - bare, primary, etc. - this could be another type).

@MatissJanis commented on GitHub (Jul 29, 2023): My recommendation: Unify `ButtonLink`, `ExternalLink` and `AnchorLink` to a single component. And move `LinkButton` inside `button` with a new `type` (`Button` supports various styles - bare, primary, etc. - this could be another `type`).
GiteaMirror commented 2026-02-28 19:07:51 -06:00
Author
Owner
Copy Link

@j-f1 commented on GitHub (Jul 29, 2023):

Unify ButtonLink, ExternalLink and AnchorLink to a single component.

These are all pretty different components despite the similar name so I’m not sure this makes sense.

  • ButtonLink has a completely different visual style than the others since it looks like a button instead of a link.
  • AnchorLink is confusingly named and really only needed in the sidebar (its other usage sites should be replaced with a plain Link). Its job is to change appearance when activated (this is how the current item in the sidebar turns purple). It should probably either be renamed or inlined into the ItemContent component. It doesn’t even end up really looking like a normal link.
  • ExternalLink is specifically for external links, ie not ones that go through React Router. I think being explicit about that is a good thing.

And move LinkButton inside button with a new type (Button supports various styles - bare, primary, etc. - this could be another type).

Until recently, LinkButton just used Button and overrode it with styles. This had some strange effects, but I think merging it with Button directly could help since all the relevant code affecting its appearance is in one place.

@j-f1 commented on GitHub (Jul 29, 2023): > Unify `ButtonLink`, `ExternalLink` and `AnchorLink` to a single component. These are all pretty different components despite the similar name so I’m not sure this makes sense. - `ButtonLink` has a completely different visual style than the others since it looks like a button instead of a link. - `AnchorLink` is confusingly named and really only needed in the sidebar (its other usage sites should be replaced with a plain `Link`). Its job is to change appearance when activated (this is how the current item in the sidebar turns purple). It should probably either be renamed or inlined into the `ItemContent` component. It doesn’t even end up really looking like a normal link. - `ExternalLink` is specifically for _external_ links, ie not ones that go through React Router. I think being explicit about that is a good thing. > And move `LinkButton` inside `button` with a new `type` (`Button` supports various styles - bare, primary, etc. - this could be another `type`). Until recently, `LinkButton` just used `Button` and overrode it with styles. This had some strange effects, but I think merging it with Button directly could help since all the relevant code affecting its appearance is in one place.
GiteaMirror commented 2026-02-28 19:07:51 -06:00
Author
Owner
Copy Link

@MatissJanis commented on GitHub (Jul 29, 2023):

Conceptually they are all the same thing though: links. Maybe the underlying logic or styling is a bit different (external link vs internal react-router link), but conceptually they are the same thing.

Think of it from a newcomers perspective. I (newcomer) want to add a new link. Should I use ButtonLink? Or LinkButton? Or perhaps AnchorLink? I don't know.

In an ideal world we would have a single shared component (highlight on the "shared" aspect; it's fine to have multiple variations internally, but not on the "shared" layer) for links that does some magic internally to convert it to button/a href/external link/etc.

(code snippet purely for illustration)

<Link to="/rules">Rules</Link>
<Link to="https://actualbudget.com" type="button">Docs</Link>
<Link to="/schedules" style={isActive ? activeStyle : inactiveStyle}>Schedules in nav</Link>
@MatissJanis commented on GitHub (Jul 29, 2023): Conceptually they are all the same thing though: links. Maybe the underlying logic or styling is a bit different (external link vs internal react-router link), but conceptually they are the same thing. Think of it from a newcomers perspective. I (newcomer) want to add a new link. Should I use `ButtonLink`? Or `LinkButton`? Or perhaps `AnchorLink`? I don't know. In an ideal world we would have a single **shared component** (highlight on the "shared" aspect; it's fine to have multiple variations internally, but not on the "shared" layer) for links that does some magic internally to convert it to button/a href/external link/etc. (code snippet purely for illustration) ``` <Link to="/rules">Rules</Link> <Link to="https://actualbudget.com" type="button">Docs</Link> <Link to="/schedules" style={isActive ? activeStyle : inactiveStyle}>Schedules in nav</Link> ```
GiteaMirror commented 2026-02-28 19:07:52 -06:00
Author
Owner
Copy Link

@th3c0d3br34ker commented on GitHub (Sep 10, 2023):

Hi @MatissJanis, I would like to take up this issue. Since it will require major refactoring, before starting, I wanted to have a quick discussion.
Here is what I plan to do:

  • move ButtonLink, ExternalLink and AnchorLink to a single component called Link
  • based on the type property (default: link) different components will be rendered.
  • create a PR where I'll be posting the structure of the new Link component.

Please let know if your thoughts have changed on refactoring the Link component. Any suggestions are also appreciated.

@th3c0d3br34ker commented on GitHub (Sep 10, 2023): Hi @MatissJanis, I would like to take up this issue. Since it will require major refactoring, before starting, I wanted to have a quick discussion. Here is what I plan to do: - move `ButtonLink`, `ExternalLink` and `AnchorLink` to a single component called `Link` - based on the `type` property (default: `link`) different components will be rendered. - create a PR where I'll be posting the structure of the new `Link` component. Please let know if your thoughts have changed on refactoring the Link component. Any suggestions are also appreciated.
GiteaMirror commented 2026-02-28 19:07:52 -06:00
Author
Owner
Copy Link

@MatissJanis commented on GitHub (Sep 10, 2023):

That sounds good. My recommendation would also be to first create this new
component and then move only 3x existing buttons/links to the new version
(all in a single PR). And then once we get the initial Link component
approved and merged - slowly migrate all the old ones to the new format.

We try to avoid massive PRs as they have a high risk of breaking things.

On Sun 10 Sep 2023 at 16:00, Jainam Desai @.***> wrote:

Hi @MatissJanis https://github.com/MatissJanis, I would like to take up
this issue. Since it Before starting, I wanted to have a quick discussion.
Here is what I plan to do:

  • move ButtonLink, ExternalLink and AnchorLink to a single component
    called Link
  • based on the type property (default: link) different components will
    be rendered.
  • create a PR where I'll be posting the structure of the new Link
    component.

Please let know if your thoughts have changed on refactoring the Link
component. Any suggestions are also appreciated.

—
Reply to this email directly, view it on GitHub
https://github.com/actualbudget/actual/issues/1414#issuecomment-1712836795,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AAGYOJ7XJUMG6OZJUV3WFVDXZXIYFANCNFSM6AAAAAA24NTVLA
.
You are receiving this because you were mentioned.Message ID:
@.***>

@MatissJanis commented on GitHub (Sep 10, 2023): That sounds good. My recommendation would also be to first create this new component and then move only 3x existing buttons/links to the new version (all in a single PR). And then once we get the initial Link component approved and merged - slowly migrate all the old ones to the new format. We try to avoid massive PRs as they have a high risk of breaking things. On Sun 10 Sep 2023 at 16:00, Jainam Desai ***@***.***> wrote: > Hi @MatissJanis <https://github.com/MatissJanis>, I would like to take up > this issue. Since it Before starting, I wanted to have a quick discussion. > Here is what I plan to do: > > - move ButtonLink, ExternalLink and AnchorLink to a single component > called Link > - based on the type property (default: link) different components will > be rendered. > - create a PR where I'll be posting the structure of the new Link > component. > > Please let know if your thoughts have changed on refactoring the Link > component. Any suggestions are also appreciated. > > — > Reply to this email directly, view it on GitHub > <https://github.com/actualbudget/actual/issues/1414#issuecomment-1712836795>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AAGYOJ7XJUMG6OZJUV3WFVDXZXIYFANCNFSM6AAAAAA24NTVLA> > . > You are receiving this because you were mentioned.Message ID: > ***@***.***> >
GiteaMirror commented 2026-02-28 19:07:52 -06:00
Author
Owner
Copy Link

@th3c0d3br34ker commented on GitHub (Sep 23, 2023):

hey @MatissJanis, I am glad to that I could contribute.

The remaining task is to add ExternalLink component. Will be raising a PR for the same soon.

I wanted to ask about -- when/how do we address the usage of old components?

@th3c0d3br34ker commented on GitHub (Sep 23, 2023): hey @MatissJanis, I am glad to that I could contribute. The remaining task is to add `ExternalLink` component. Will be raising a PR for the same soon. I wanted to ask about -- when/how do we address the usage of old components?
GiteaMirror commented 2026-02-28 19:07:53 -06:00
Author
Owner
Copy Link

@MatissJanis commented on GitHub (Sep 23, 2023):

I wanted to ask about -- when/how do we address the usage of old components?

Up to you: if you want to do it, then feel free to perform the migration over a single (or multiple smaller) PRs. If not - I'd recommend adding a @deprecated comment to the old components with instructions how to port over to the new ones. And then hopefully someone else picks it up.

@MatissJanis commented on GitHub (Sep 23, 2023): > I wanted to ask about -- when/how do we address the usage of old components? Up to you: if you want to do it, then feel free to perform the migration over a single (or multiple smaller) PRs. If not - I'd recommend adding a `@deprecated` comment to the old components with instructions how to port over to the new ones. And then hopefully someone else picks it up.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
AI generated
API
bank sync
budgeting
bug
can’t replicate
dependencies
docker
documentation
electron
experimental feature
feature
feedback
goal templates
good first issue
help wanted
importers
maintenance
needs info
needs testing
needs triage
needs votes
openid
payees
pull-request
Mirrored from GitHub Pull Request
 regression
reports
responsive
rules
schedules
server
✨ merged
split transactions
tech debt
theme
transaction import
transaction reconciliation
transactions
translations
upstream
user interface
✅ approved
wontfix
No Label good first issue tech debt
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
GiteaMirror ninjasurge
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: github-starred/actual#530
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?