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

Make Builder App Section Navigation Tabs Anchors #13626

Merged
merged 11 commits into from May 21, 2024
Merged

Conversation

Ghrehh
Copy link
Collaborator

@Ghrehh Ghrehh commented May 7, 2024

A lot of our links throughout the app aren't actual anchors, but are buttons that behave like links.

This isn't great for a few reasons, but one particular frustrating one for me was being unable to open the data section in a new tab from the design section while working on an app.

This change makes the builder app editing navigation actual links, while still maintaining the onClick functionality of the tab component.

If there's no issues with this we can easy do it for any other consumers of the tab component, and the same sort of change can be replicated across other link buttons in the app.

Copy link
Contributor

@deanhannigan deanhannigan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is such a great UX enhancement! Makes a massive difference especially when popping open the app list in another tab 🎉

@aptkingston
Copy link
Member

aptkingston commented May 13, 2024

Another way to handle this (which is applicable to all sorts of components without much in the way of code changes) would just be to detect the user holding down ctrl/cmd and then open a new tab.

The only changes required for any component like this is inside the on:click handler:

    if (e.ctrlKey || e.metaKey) {
      window.open($url(path), "_blank")
    } else {
      $goto(path)
    }

We also just need to ensure we have access to the native JS event parameter, and the best way to do that (if you already have a custom handler for on:click like we do in the Tab component) is add another on:click just to proxy it up.

So in the Tab component we can remove the custom dispatch("click") and instead just do

    on:click={onClick}
    on:click

That ensures that the on:click callback in the parent has the proper event param.

The main reason I prefer this approach is that it would work for absolutely any component with a click handler without having to add new markup for HTML link components into each of them.

@Ghrehh
Copy link
Collaborator Author

Ghrehh commented May 15, 2024

@aptkingston I understand where you're coming from, but the implementation you suggested doesn't do the following (I think):

  • On hover link previews.
  • Right-click link menu (or force touch on mobile etc)
  • Semantic html stuff for screen readers/search engine indexing etc
  • Probably some other stuff I'm not aware of.

I'm sure we could implement most if not all of that in JS, but at some point I think it makes sense to just use an anchor if it's anchor behaviour that we want/is expected 🤷

Though I also get that this is a change to a very important component for some pretty minor convenience/tidiness gains, so I'm happy to change it to your suggested implementation if you think the reduction to the risk of breaking something is worth the compromise.

@Ghrehh Ghrehh force-pushed the make-make-nav-anchor-tags branch from f63b165 to 3c8d296 Compare May 15, 2024 08:16
@aptkingston
Copy link
Member

You're not wrong that it doesn't address those accessibility issues, but I was just going off your description where the primary motivation was being able to open links in new tabs. The builder doesn't support mobile either, but yea I'm happy to just leave this as it is then to address those extra points you mentioned.

@Ghrehh Ghrehh merged commit c394826 into master May 21, 2024
10 checks passed
@Ghrehh Ghrehh deleted the make-make-nav-anchor-tags branch May 21, 2024 09:22
@github-actions github-actions bot locked and limited conversation to collaborators May 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants