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 URL display style #14

Merged
merged 1 commit into from Oct 10, 2018
Merged

Conversation

marcosnils
Copy link
Contributor

@marcosnils marcosnils commented Oct 10, 2018

- Removes unnecesary function
- Fixes alexweininger#13
@alexweininger
Copy link
Owner

alexweininger commented Oct 10, 2018

@marcosnils
Thanks for the pr!

Can you provide a screenshot of the popup using your code?

One of the reasons I had

file.filename.substring(0, file.filename.lastIndexOf('\\') + 1)

instead of

linkUrl.innerHTML = file.filename;

was to make sure the displayed url was not too long and messy. I wanted to do this because 1. I want it to fit on one line, and 2. I want it to be easy to read.

I am worried that using the full file path won't meet these. I know I should have put this in the original issue, sorry.

Do you have any ideas of how to solve this problem?

-alex

@marcosnils
Copy link
Contributor Author

image

This is how it looks in my machine. The problem was that several filenames didn't have the \\ at all in the path so that line was causing the url not to render at all.

What you can do if you don't want the path to mess with the length is truncate the url with ellipsis using css (https://css-tricks.com/snippets/css/truncate-string-with-ellipsis/)

@alexweininger
Copy link
Owner

alexweininger commented Oct 10, 2018

I like the ellipsis idea however I think the most useful information in the filepath is at the end. I would like something like a reverse ellipse, so that we always see the end of the path and the front gets cut off if the path is too long.

Also - all the file paths begin with "file://" if I can recall correctly. So the issue is something else. See the updates issue #13.

@alexweininger alexweininger added good first issue Good for newcomers help wanted Extra attention is needed labels Oct 10, 2018
@alexweininger alexweininger self-assigned this Oct 10, 2018
@alexweininger alexweininger self-requested a review October 10, 2018 09:23
@alexweininger alexweininger removed their assignment Oct 10, 2018
@marcosnils
Copy link
Contributor Author

Also - all the file paths begin with "file://" if I can recall correctly. So the issue is something else. See the updates issue #13.

That wasn't my case. The issue about url's not appearing was because a styling issue that I also fixed in the PR. The URL's are there, it's just that the CSS styles were causing the file name to overlap them. You can try merging this PR locally in your machine and check out how it works.

@alexweininger
Copy link
Owner

alexweininger commented Oct 10, 2018

You can try merging this PR locally in your machine and check out how it works.

Will do👍

@alexweininger alexweininger merged commit b74a284 into alexweininger:master Oct 10, 2018
@alexweininger
Copy link
Owner

@marcosnils just checked it out, looks great! Sorry for my pestering.

I will note that there are further improvements from this that need to be made. I will link the issues here later.

Copy link
Owner

@alexweininger alexweininger 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!

@marcosnils
Copy link
Contributor Author

@marcosnils just checked it out, looks great! Sorry for my pestering.

No worries, glad to help! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants