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

Opacities action #1260

Merged
merged 102 commits into from
Feb 7, 2023
Merged

Opacities action #1260

merged 102 commits into from
Feb 7, 2023

Conversation

themacboy
Copy link
Contributor

@themacboy themacboy commented Oct 30, 2022

Fixes #311

First of all, sorry for my poor English and weak coding skills (I do it just for fun!)

I was working on a new action to set custom and variable opacity.

Please check the video for more details:

Opacities Action

You can play it:

Opacities Action example page

@themacboy
Copy link
Contributor Author

Does anyone like this code? haha

I feel lonely! ;)

@vanithaak
Copy link
Contributor

Does anyone like this code? haha

I feel lonely! ;)

Hey! I like your code. Nice work 👏

@7malikk
Copy link
Collaborator

7malikk commented Nov 15, 2022

@themacboy Its great!!
I went through it before you closed the PR the first time.
I quite like the idea
@jywarren should check it out eventually

PS. glad you reopened it 🙌🏾🙌🏾

@themacboy
Copy link
Contributor Author

I'm currently using CSSStyleSheet to define my css overrides... but that's too modern and not all browsers have a correct implementation.

Then I'm not sure about changing it to the traditional <style> tag.

@alenigrelli
Copy link

can you merge in the project?. Thanks!

Copy link
Member

@jywarren jywarren left a comment

Choose a reason for hiding this comment

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

Hi, sorry for my slow review here, been busy! Much appreciated, this looks really cool, and I love the new icon!

Just to clarify, is this on by default in one of the examples? Also, a Q below about the CDN leaflet. We could upgrade Leaflet itself if that's preferable to switching to a CDN.

@@ -6,12 +6,12 @@
<meta name="viewport" content="width=device-width, initial-scale=1.0, maximum-scale=1.0, user-scalable=no"/>
<meta http-equiv="x-ua-compatible" content="ie=edge"/>

<link rel="stylesheet" href="../node_modules/leaflet/dist/leaflet.css">
<link rel="stylesheet" href="https://cdnjs.cloudflare.com/ajax/libs/leaflet/1.9.2/leaflet.min.css">
Copy link
Member

Choose a reason for hiding this comment

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

Here, i'm not against pegging a specific version via CDN, but was this necessary to make this work? Just curious if we could remove this to simplify the PR. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello ! Im alive !!!

Thx for your all yout comments !

The reason for moving to CDN was due to Github Pages service is failing when trying to load local files (missing files).

image

Then I couldn't test my changes on the web, lol, that's the only reason for this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If needed I can set local method before the final merge ;)

Copy link
Member

Choose a reason for hiding this comment

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

Oh that would be great to revert this back to the local one. We use that in our gh-pages demo. Thank you!

<link rel="stylesheet" href="../dist/vendor.css">
<link rel="stylesheet" href="../dist/leaflet.distortableimage.css" media="screen" title="no title">
<link rel="stylesheet" href="https://unpkg.com/leaflet-control-geocoder/dist/Control.Geocoder.css" />

<script src="../node_modules/leaflet/dist/leaflet.js" type="text/javascript" charset="utf-8"></script>
<script src="https://cdnjs.cloudflare.com/ajax/libs/leaflet/1.9.2/leaflet.js" type="text/javascript" charset="utf-8"></script>
Copy link
Member

Choose a reason for hiding this comment

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

Same here. Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

And here too!

@jywarren
Copy link
Member

Also, THANK YOU for such great video documentation, live example, tidy code, and adjustments to the tests!

@jywarren
Copy link
Member

And just one final question - is this on by default in one of the examples? Thanks!

@jywarren
Copy link
Member

(i see you adjusted the CDNs so otherwise approved! Thanks!)

@themacboy
Copy link
Contributor Author

@jywarren update with updating new commits.

@themacboy themacboy closed this Feb 6, 2023
@themacboy themacboy reopened this Feb 6, 2023
@jywarren
Copy link
Member

jywarren commented Feb 6, 2023

Thanks!! Just wanted to know that last question -- is this on by default in one of the examples? Thanks!

@themacboy
Copy link
Contributor Author

Thanks!! Just wanted to know that last question -- is this on by default in one of the examples? Thanks!

yes ! I think it is.

@jywarren jywarren merged commit 9472b3f into publiclab:main Feb 7, 2023
@jywarren
Copy link
Member

jywarren commented Feb 7, 2023

Wonderful, thank you!!!!!

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

Successfully merging this pull request may close these issues.

Get the transparency tool to work on a range
5 participants