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

Fix disabled links refresh page #132

Merged
merged 6 commits into from
Jul 8, 2021
Merged

Fix disabled links refresh page #132

merged 6 commits into from
Jul 8, 2021

Conversation

hoektoe
Copy link
Contributor

@hoektoe hoektoe commented Jul 6, 2021

Why are these changes introduced?

Fixes #8.

Affected components are:

  • image-with-text
  • image-banner

What approach did you take?
Components with anchor links that could be empty, check for blank URL and remove the Href from the anchor tag. This approach still shows the end-user disabled cursor as per CSS styling for aria-disabled

Hovering over anchors without Href shows end-user disabled cursor and does nothing when clicked

Other considerations

  • Pointer-events: none, this approach nullified the cursor CSS and unwanted consequences
  • javascript:void(0) , this approach could also work, but since removing Href is HTML standard it's probably better.

Demo links

Checklist

@ghost ghost added the cla-needed label Jul 6, 2021
@ghost ghost removed the cla-needed label Jul 6, 2021
bertiful
bertiful previously approved these changes Jul 7, 2021
Copy link
Contributor

@bertiful bertiful left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @hoektoe. Small suggestion to remove some extra whitespace otherwise we're good 👍🏻

sections/image-banner.liquid Outdated Show resolved Hide resolved
sections/image-banner.liquid Outdated Show resolved Hide resolved
sections/image-with-text.liquid Outdated Show resolved Hide resolved
tyleralsbury
tyleralsbury previously approved these changes Jul 7, 2021
Copy link
Contributor

@tyleralsbury tyleralsbury left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! Great idea, and it works well. ✅

As Chris mentioned, a little whitespace cleanup, but otherwise looks great.

@hoektoe hoektoe dismissed stale reviews from tyleralsbury and bertiful via 321df96 July 8, 2021 07:03
hoektoe and others added 3 commits July 8, 2021 09:03
Co-authored-by: Chris Berthe <chris.berthe@shopify.com>
Co-authored-by: Chris Berthe <chris.berthe@shopify.com>
Co-authored-by: Chris Berthe <chris.berthe@shopify.com>
@hoektoe
Copy link
Contributor Author

hoektoe commented Jul 8, 2021

Thanks, @chrisberthe , applied the suggestions. Didn't know could leave a code suggestion like that and have it applied/accepted like that.

Hope all is well?

Copy link
Contributor

@bertiful bertiful left a comment

Choose a reason for hiding this comment

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

Yeah, nifty little GitHub feature. Something else that's pretty cool is that you can look at the "Files changed" tab and batch the suggestions together into a single commit instead of committing each suggestion separately.

Code looks good btw! We'll just wait on @tyleralsbury to review the latest. Thanks for your contribution :)

Copy link
Contributor

@tyleralsbury tyleralsbury left a comment

Choose a reason for hiding this comment

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

Looks good - tested and code ✅

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

Successfully merging this pull request may close these issues.

Disabled links refresh the page
3 participants