accordproject / web-components

React Components for Accord Project
Apache License 2.0
120 stars 101 forks source link

Hyperlink modal goes off page #219

Closed Michael-Grover closed 3 years ago

Michael-Grover commented 4 years ago

Bug Report 🐛

The hyperlink modal sometimes goes off the page if the link is near the edge of the page. image

I may provide a mockup for how this could be fixed later, but open to suggestions

nik72619c commented 4 years ago

Anyone working on this @Michael-Grover? Would like to take this up 🙂

K-Kumar-01 commented 3 years ago

@Michael-Grover The issue seems inactive to me. I would like to have a go at the issue. My Possible solutions: 1) Change the style property of the box (we would also need to change the arrow above the link box). 2) Change the width of the box and set overflow-x to scroll.

You have suggested providing a mockup, so I am open to that also.

jolanglinais commented 3 years ago

@K-Kumar-01 go for it! I think option 1 might be best to start with.

Michael-Grover commented 3 years ago

I agree, thanks @K-Kumar-01

K-Kumar-01 commented 3 years ago

@Michael-Grover @irmerk I have made the changes. Had a doubt regarding the point

Before creating the Pull Request, ensure your branch sits on top of master (as opposed to branch off a branch). This ensures the reviewer will need only minimal effort to integrate your work by fast-fowarding master:

Since I cloned latest repo and there were no changes made so I did not do the following:

    git checkout master
    git fetch --all --prune
    git rebase upstream/master
    git push origin master

Now while doing git rebase upstream/master it is showing invalid upstream. Since the repo is already at latest, Can i proceed without this step?

Also on npm run test it shows missing script:doc P.S:Although as far as I think the test should not fail as only styling is changed.

jolanglinais commented 3 years ago

@K-Kumar-01 those instructions are for forking this repository. Since you just cloned it, you can probably do the following:

    git checkout master
    git pull
    git checkout <YOUR_BRANCH>
    git rebase origin/master
    git push <YOUR_BRANCH> master

I'd encourage you to fork the repository in the future though!

K-Kumar-01 commented 3 years ago

@K-Kumar-01 those instructions are for forking this repository. Since you just cloned it, you can probably do the following:

    git checkout master
    git pull
    git checkout <YOUR_BRANCH>
    git rebase origin/master
    git push <YOUR_BRANCH> master

I'd encourage you to fork the repository in the future though!

I had forked the repo. There seems to be a misunderstanding. It's just that I think there is no rebasing required as no changes were made when I forked so the above comment. I apologize for the bad choice of words I made earlier. My main doubt is the rebase part.

jolanglinais commented 3 years ago

Oh oops, okay so when you cloned your fork, did you set the upstream? git remote add upstream "https://github.com/accordproject/web-components.git" That could cause this issue:

Now while doing git rebase upstream/master it is showing invalid upstream.

Still, yeah you should be able to push up and open a PR fine.

K-Kumar-01 commented 3 years ago

Oh oops, okay so when you cloned your fork, did you set the upstream? git remote add upstream "https://github.com/accordproject/web-components.git" That could cause this issue:

Now while doing git rebase upstream/master it is showing invalid upstream.

Still, yeah you should be able to push up and open a PR fine.

Nope, I did not add upstream. So should I make PR? Done the following steps:

  1. Made a new branch
  2. Made a commit

Got an error in the upstream part, so had a doubt.

jolanglinais commented 3 years ago

Not 100% sure, but I believe you don't need to add the upstream in order for GitHub to pick up your pushed commit and allow you to open a PR. So yeah just make the PR.

K-Kumar-01 commented 3 years ago

@irmerk @Michael-Grover I have made a PR for the issue. Few things to clarify. npm run test gave an error so testing not done (though only styling is changed) The process was brute force by searching for the breakpoints manually. Further devices less than 335px won't have modal arrow rendered correctly(though modal will render correctly). Please let me know if any changes are required.