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

Should add comment to imply what browser uses what function #8

Closed
alystair opened this issue Jul 1, 2010 · 5 comments
Closed

Should add comment to imply what browser uses what function #8

alystair opened this issue Jul 1, 2010 · 5 comments

Comments

@alystair
Copy link

alystair commented Jul 1, 2010

Right now looking at the code I have to guess which browser was implementing what code in the source. Would be nice to add comment per segment for ease of future modifications.

@marcuswestin
Copy link
Owner

Hey alystair,

That would be a great! I'd definitely pull a patch.

Thanks for the suggestions.

@alystair
Copy link
Author

alystair commented Jul 1, 2010

Actually I was hoping that you could add the commenting (since you originally created the script and know exactly what browser does what [hopefully]), so I could focus on index/length/push/pop :)

@marcuswestin
Copy link
Owner

Ah, I see. Well, the implementation is based on feature detection and I don't believe comments of which browser supports what method is necessary. For example, I don't see how implementation of a feature like index/length/push/pop would depend on which browser it is in if you can assume that the underlying get/set api works.

If there is a web page with a table of support out there than linking to that in the readme could definitely be useful. I would very much appreciate if you took the time to find or make one!

Cheers,
Marcus

@marcuswestin
Copy link
Owner

Considering the implementation is feature detection based rather than browser detection, I don't think this is a relevant issue. Closing. Feel free to reopen with additional info/comments if you think it's importan.t

@marcuswestin
Copy link
Owner

.

This issue was closed.
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

No branches or pull requests

2 participants