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: remove nested links and buttons #2818

Merged
merged 2 commits into from
Jun 13, 2022

Conversation

Olaleye-Blessing
Copy link
Contributor

Signed-off-by: Olaleye Blessing [email protected]

Description
Fix wrongly nested links and buttons.
This PR fixes #2740

Notes for Reviewers
Still needs to update components that're affected by these changes.

Signed commits

  • Yes, I signed my commits.

@Olaleye-Blessing Olaleye-Blessing marked this pull request as ready for review May 24, 2022 10:33
@Olaleye-Blessing Olaleye-Blessing marked this pull request as draft May 24, 2022 10:43
@Olaleye-Blessing
Copy link
Contributor Author

I was planning to fix some layouts affected by this change but I can't cause I'm having a weird error while trying to run the site locally. This started today.

Screenshot (556)

One obvious affected layout is the screenshot below:

Screenshot (557)_LI

the paragraph/text in each article is shifted downwards

@Olaleye-Blessing
Copy link
Contributor Author

@leecalcote

@leecalcote
Copy link
Member

I missed your ping here, @Olaleye-Blessing. @YashKamboj will you have a look at this?

@leecalcote leecalcote requested a review from YashKamboj June 6, 2022 23:53
@l5io
Copy link
Contributor

l5io commented Jun 7, 2022

🚀 Preview for commit a9ba56f at: https://629e95ad8c09091969e7509b--layer5.netlify.app

@YashKamboj
Copy link
Contributor

@Olaleye-Blessing Are you still facing the errors, as i can see that you are using windows, why don't you use git bash to use make commands or wsl

Copy link
Contributor

@YashKamboj YashKamboj left a comment

Choose a reason for hiding this comment

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

You did a great work , but i want to ask that why don't you use the simple solution like adding <form> around the button, it would a simple fix

@Olaleye-Blessing
Copy link
Contributor Author

@Olaleye-Blessing Are you still facing the errors, as i can see that you are using windows, why don't you use git bash to use make commands or wsl

I formatted my laptop some weeks ago. I will try again and see if the error is gone.

@Olaleye-Blessing
Copy link
Contributor Author

You did a great work , but i want to ask that why don't you use the simple solution like adding <form> around the button, it would a simple fix

I don't understand this. Could you explain further? I think what I'm trying to do is to prevent having a <button> being nested inside a <a>.

@YashKamboj
Copy link
Contributor

@Olaleye-Blessing i meant something like this

<form action="http://example.com/" method="get">
  <button>Visit Website</button>
</form>

I also found something else too , not sure if it will work , give this a try too
<button type="button" onclick="location.href='http://www.stackoverflow.com'">ABC</button>

@Olaleye-Blessing
Copy link
Contributor Author

@YashKamboj true, the form will work. But don't you think screen readers are going to have a problem?

According to css trick article, there should be a difference when using a link and a button. Using a button with onClick will show a button even for links.

@Olaleye-Blessing
Copy link
Contributor Author

And I think my drafted pull request has fixed the issue, the only thing left is to correct the layouts that have been affected.

@Olaleye-Blessing
Copy link
Contributor Author

@leecalcote @YashKamboj. Like I said earlier, the issue has been fixed but some layouts are affected. Should the layouts be fixed in this pull request?

@YashKamboj
Copy link
Contributor

@Olaleye-Blessing i gues @Nikhil-Ladha could give you a better review on this one

@l5io
Copy link
Contributor

l5io commented Jun 13, 2022

🚀 Preview for commit f54ac69 at: https://62a6d04434343a770f57aa70--layer5.netlify.app

@Nikhil-Ladha
Copy link
Contributor

Everything else seems fine, just that box-shadow effect on the news section on the home page needs to be fixed.
Can you please take care of that @Olaleye-Blessing ?

@Nikhil-Ladha
Copy link
Contributor

That is, adding a box-shadow: none after this line will fix the issue.

@Nikhil-Ladha
Copy link
Contributor

Please follow the instruction mentioned here to fix the DCO check

Olaleye-Blessing and others added 2 commits June 13, 2022 16:06
@Olaleye-Blessing
Copy link
Contributor Author

@Nikhil-Ladha done

@l5io
Copy link
Contributor

l5io commented Jun 13, 2022

🚀 Preview for commit 16fe509 at: https://62a759b38cadbf0651423acc--layer5.netlify.app

@Nikhil-Ladha Nikhil-Ladha marked this pull request as ready for review June 13, 2022 15:44
Copy link
Contributor

@Nikhil-Ladha Nikhil-Ladha 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.

@Nikhil-Ladha Nikhil-Ladha merged commit c8c782a into layer5io:master Jun 13, 2022
@leecalcote
Copy link
Member

Uh-oh. There seems to be an adverse effect here of the Saffron (yellow) color button style going missing. Let's revert this PR to immediately get the styling returned and then look to have this PR's changes re-implemented.

@Nikhil-Ladha
Copy link
Contributor

Uh-oh. There seems to be an adverse effect here of the Saffron (yellow) color button style going missing. Let's revert this PR to immediately get the styling returned and then look to have this PR's changes re-implemented.

Where did you see this effect? I couldn't find it 🤔

@Nikhil-Ladha
Copy link
Contributor

Ok, now I see what went wrong.
The buttons having internal link got affected. @Olaleye-Blessing can you take a look and fix it?

@Olaleye-Blessing
Copy link
Contributor Author

ok, I'll look into it.

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

Successfully merging this pull request may close these issues.

Wrong nested interactive element.
5 participants