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

URLs being auto-added, CSS #145

Open
dxdc opened this issue Aug 20, 2020 · 6 comments
Open

URLs being auto-added, CSS #145

dxdc opened this issue Aug 20, 2020 · 6 comments

Comments

@dxdc
Copy link

dxdc commented Aug 20, 2020

Great extension! A few questions/comments that would be really helpful to clarify from the docs:

  1. Is it necessary to include JS in the following kind of immediately invoked expression and/or use .noConflict if jQuery libraries are included?

e.g.,

jQuery.noConflict();
(function( $ ) {
    // Your jQuery code here
})( jQuery );
  1. There is a CSS/JS select combobox in the panel, but it seems the CSS box doesn't do anything? It would be great to be able to add styles and JS for each page if that is true, but otherwise, I'm not sure the point of it? However, this also works, of course...
$("head").append('<style type="text/css">' + css + "</style>");
  1. When a user clicks the extension, it automatically adds the URL/domain name into the list of options. I think it should only be added if there is any code added/modified?

Related, but when Reset is clicked, it automatically refreshes the page without any user confirmation. I think it would be helpful to just provide a prompt to confirm the user wants to refresh the page.

  1. Is there a way to also specify that the code should run for a particular path, not just domain? Is customjsReady an alternative for this (so, it could only run when a particular element is detected?)

  2. Would be great if you can add a Wiki to this project. Then, users could post examples of their own JS/working examples.

@xcv58
Copy link
Owner

xcv58 commented Aug 20, 2020

@dxdc Thanks for your feedbacks/questions.

  1. Is it necessary to include JS in the following kind of immediately invoked expression and/or use .noConflict if jQuery libraries are included?

I think it would be overkill to customize for a library.

  1. There is a CSS/JS select combobox in the panel, but it seems the CSS box doesn't do anything? It would be great to be able to add styles and JS for each page if that is true, but otherwise, I'm not sure the point of it? However, this also works, of course...

It has nothing to do with CSS but only css syntax highlight, #58

  1. When a user clicks the extension, it automatically adds the URL/domain name into the list of options. I think it should only be added if there is any code added/modified?

It's definitely doable. Not sure whether there is any drawback of this. I tend to not do this because there aren't a lot of benefits. I would suggest using this feature to easily manage the host list:
#90

And it seems there is a bug for navigation links. I can fix it.

Related, but when Reset is clicked, it automatically refreshes the page without any user confirmation. I think it would be helpful to just provide a prompt to confirm the user wants to refresh the page.

This one can easily be added. I think we can expand the buttons on the current popup to three: Cancel, Reset, and Reset & Refresh.

  1. Is there a way to also specify that the code should run for a particular path, not just domain? Is customjsReady an alternative for this (so, it could only run when a particular element is detected?)

It's not possible per current implementation. Users can implement this logic by if else branch in the code itself.

The current approach to store/sync the data/code doesn't scale. Adding more logic would cause more issues. It's blocked by #32

  1. Would be great if you can add a Wiki to this project. Then, users could post examples of their own JS/working examples.

I just enabled the Wiki feature for this repo: https://github.com/xcv58/Custom-JavaScript-for-Websites-2/wiki

@dxdc
Copy link
Author

dxdc commented Aug 20, 2020

@xcv58 thanks for the quick response!

I think it would be overkill to customize for a library.

I didn't quite get you on that. Did you mean that it isn't being done? I guess I just wonder how it should be used. E.g., what happens if jQuery extension is already loaded by that page? Are namespace conflicts already sorted out by the plugin or should the user include this?

BTW... this is in reference just to the pre-loaded list of libraries (jQuery, etc.)

It has nothing to do with CSS but only css syntax highlight, #58

OK. In that case, would be useful to add some kind of warning to the user that this is just for syntax highlighting. Maybe I'm missing the point, but I don't understand how/why this is helpful at all, if not just confusing.

It's definitely doable. Not sure whether there is any drawback of this. I tend to not do this because there aren't a lot of benefits. I would suggest using this feature to easily manage the host list:
#90

Yes, the management is useful, but if someone clicks on the extension on the page (for ex., to view another script) it will automatically add that domain name. I just think it could be more user-controlled as opposed to auto-added? It's not intuitive to me at least why simply viewing a page would create a new host entry.

And it seems there is a bug for navigation links. I can fix it.

Great!

This one can easily be added. I think we can expand the buttons on the current popup to three: Cancel, Reset, and Reset & Refresh.

Makes a lot of sense. Great!

It's not possible per current implementation. Users can implement this logic by if else branch in the code itself.

Maybe I missed your point, but I didn't mean anything to do with the storage location of the JS. I mean path of the URL parameter. E.g., example.com/path1/ vs example.com/path2/

This can of course be analyzed by some parameter like window.location. Since we can run JS we can differentiate it within the code itself, but would be cool to also define which paths specifically to apply the JS. (E.g., just on the login page, facebook messenger page, etc.)

I just enabled the Wiki feature for this repo: https://github.com/xcv58/Custom-JavaScript-for-Websites-2/wiki

Awesome. Will try to post something there once I refine my ideas a bit more. Thanks again for your responses.

@xcv58
Copy link
Owner

xcv58 commented Aug 20, 2020

I didn't quite get you on that. Did you mean that it isn't being done? I guess I just wonder how it should be used. E.g., what happens if jQuery extension is already loaded by that page? Are namespace conflicts already sorted out by the plugin or should the user include this?
BTW... this is in reference just to the pre-loaded list of libraries (jQuery, etc.)

I mean this extension isn't tied to jQuery or any other JS library. To optimize for a JS library inside this extension seems to be overkill because we can't optimize for every external JS library user might use.

And it's on user's own decision whether to inject a library. They should be aware of whether they need to inject jQuery or any other library on a certain webpage they worked on.

OK. In that case, would be useful to add some kind of warning to the user that this is just for syntax highlighting. Maybe I'm missing the point, but I don't understand how/why this is helpful at all, if not just confusing.

I totally agree with you. Feel free to file a PR if you want to.

Yes, the management is useful, but if someone clicks on the extension on the page (for ex., to view another script) it will automatically add that domain name. I just think it could be more user-controlled as opposed to auto-added? It's not intuitive to me at least why simply viewing a page would create a new host entry.

I just tried it. If one just opens the extension popup and do nothing. The domain would in the list but it wouldn't be added to the storage until the user input character in the editor. PS: We need to add it to the list because of the draft feature when user input something.

It has to be in the list visually to indicate what host you are currently working on. But it would be in the final list at all if user input nothing. I'm not sure whether this explains/satisfy your concerns, could you please tell me your thought and any improvement we can make?

Maybe I missed your point, but I didn't mean anything to do with the storage location of the JS. I mean path of the URL parameter. E.g., example.com/path1/ vs example.com/path2/

The current implementation needs to store all the enabled hosts and regex in one list inside the storage. And every time the page loaded, it has to iterate the entire list to check which script to load. And the storage has limitations for per item, in our case the list can't beyond a certain size (4kb if my memory is correct). So adding this feature would explode the possibility to reach the limitation. I strongly against add this with current storage implementation.

And I'm very happy to add the feature if we can have a realistic storage/API. For example, if we can just send an URL with API call and it would return a list of code snippets to execute.

This can of course be analyzed by some parameter like window.location. Since we can run JS we can differentiate it within the code itself, but would be cool to also define which paths specifically to apply the JS. (E.g., just on the login page, facebook messenger page, etc.)

There is no technical issue at all, just the storage limitation as I mentioned above.

Awesome. Will try to post something there once I refine my ideas a bit more. Thanks again for your responses.

Thank you for your feedback! And please feel free to request permission if needed to edit the Wiki page.


To recap, we have to actionable items:

  1. Fix the hosts table navgiation: And it seems there is a bug for navigation links. I can fix it.

  2. This one can easily be added. I think we can expand the buttons on the current popup to three: Cancel, Reset, and Reset & Refresh.

@dxdc
Copy link
Author

dxdc commented Aug 20, 2020

To optimize for a JS library inside this extension seems to be overkill because we can't optimize for every external JS library user might use.

Agree with you. Just wondering how the current implementation is working (for the built-in jQuery one)

I totally agree with you. Feel free to file a PR if you want to.

OK will consider it :)

But it would be in the final list at all if user input nothing. I'm not sure whether this explains/satisfy your concerns, could you please tell me your thought and any improvement we can make?

So, here's what I did. Open 2 separate tabs with different domains. On tab 1, open CJS and then immediately close it. On tab 2, open CJS and immediately close it (note: domain from tab 1 will be visible). Now, we've added both domains from tab 1 and tab 2 to the list. It just seems like it pollutes the list rather quickly.

And I'm very happy to add the feature if we can have a realistic storage/API. For example, if we can just send an URL with API call and it would return a list of code snippets to execute.

Yes, that would be fantastic. Instead of just the domain, we can make the existing regex implementation work with the whole URL. Wouldn't that be enough?

To recap, we have to actionable items:

Great!

@xcv58
Copy link
Owner

xcv58 commented Aug 20, 2020

Agree with you. Just wondering how the current implementation is working (for the built-in jQuery one)

https://github.com/xcv58/Custom-JavaScript-for-Websites-2/blob/master/src/js/run.tsx#L10

It creates a div on the fly and set the src attribute.

So, here's what I did. Open 2 separate tabs with different domains. On tab 1, open CJS and then immediately close it. On tab 2, open CJS and immediately close it (note: domain from tab 1 will be visible). Now, we've added both domains from tab 1 and tab 2 to the list. It just seems like it pollutes the list rather quickly.

I'm able to reproduce it this time. The code implement this is on:

https://github.com/xcv58/Custom-JavaScript-for-Websites-2/blob/master/src/js/stores/AppStore.tsx#L135-L136

One way to avoid this is delay this.saveHosts() to whenever users actually change the code.

Yes, that would be fantastic. Instead of just the domain, we can make the existing regex implementation work with the whole URL. Wouldn't that be enough?

It should work, I still have the same concern to put more features on top of the current implementation, and I think the let the users do the URL detection is a good tradeoff for now.

@dxdc
Copy link
Author

dxdc commented Aug 21, 2020

It creates a div on the fly and set the src attribute.

Got it. OK thanks! Makes sense. I'm having an issue with certain pages where jQuery must be already loaded and trying to load it again is causing problems. I guess implementing my own JS for that will be better, this is just a fast and flexible approach.

I'm able to reproduce it this time. The code implement this is on:

Yes, exactly. I would agree delaying this.saveHosts() would be excellent!

It should work, I still have the same concern to put more features on top of the current implementation, and I think the let the users do the URL detection is a good tradeoff for now.

Sure, understood. Thanks for the discussion!!

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