Implement visible portion features - #50

Draft
nikischin wants to merge 3 commits into
Nicolapps:mainfrom
nikischin:feature/mapVisiblePortion
Draft

Implement visible portion features#50
nikischin wants to merge 3 commits into
Nicolapps:mainfrom
nikischin:feature/mapVisiblePortion

Conversation

@nikischin

@nikischinnikischin commented Oct 2, 2023

Copy link
Copy Markdown
Contributor

Implementing features for center, rotation, region and visibleMapRect. (See #49)

Still draft as I wasn't able to fully test the rotation yet, as either storybook or my browser is a bit buggy.

@Nicolapps however, could you possible do a quick review if the general implementation seems reasonable to you?

@Nicolapps

Copy link
Copy Markdown
Owner

Thank you for your draft PR! I have a few remarks about the proposed APIs:

  • A lot of the new properties are overlapping (region, camera distance, center, visible map rect). This can lead to situations where the same parameter is specified several times, and it’s not clear what’s supposed to happen in these cases. Maybe it could be clearer to limit the number of available properties, but provide a way to convert the values between each other. Or another option could be to edit the TypeScript types to make it clear which properties are not meant to be set at the same time (maybe with something like discriminated unions).
  • It’s not possible to do bidirectional bindings of these properties with the current APIs. For instance, it is possible to set the camera distance from the user’s state, but the user doesn’t know when the camera distance changes. They could listen to the region events, but they can’t access the new value of cameraDistance without using refs.
  • MapKit JS doesn’t always emit events with the latest region values when browsing the map. When implementing bidirectional binding for these properties, I’m concerned about issues caused by the mismatch between the latest state in MapKit and the one that the user received. It would be nice to create a few stories in Storybook to show how it works in practice.
  • For some of these properties, MapKit JS overrides the value provided with a more suitable value as soon as it is set. For instance, when setting region, MapKit JS will use a region that fits the map ratio, which is most often not the case for the provided value. This could lead to a difference between the value being set and the value being applied, which works well in an imperative API like MapKit’s but could cause issues in the declarative context of React.
  • Would you want to deprecate initialRegion, as your comment in the code seems to indicate? This could still be useful for cases where the user only wants to set the initial region and wants to let the user browse the map freely without having to manage bidirectional binding.
  • What happens if the user sets region to a constant? According to React’s semantic, we probably shouldn’t let the map move (or revert the change instantly after the user is done panning?). It could be confusing if the implementation lets the set value and the actual value drift. (This is an issue we already have with the current implementation of the marker position and selected attributes, but it’s probably less confusing in this case since the ways these values can be modified are more limited.)
  • regionUpdateAnimates seems like a nice API which can make a lot of sense if the implementation works as expected, even when there is some bidirectional binding being used.

In general, I’m not sure if we could make an API like this one work well with the limitations of React and MapKit JS. But I’d love to be proved wrong and see stories that show it in action!

Out of curiosity, do you have more details about your use cases that make controlling these properties through attributes instead of refs? Using refs worked well for my project where I let the user pan freely the map and sometimes need to animate a region change to a particular location, but I understand that it might be different in your case.

@nikischin

Copy link
Copy Markdown
ContributorAuthor

Hi @Nicolapps,

thank you so much for this valuable feedback! Those are all valid points which on most I haven't yet have a clear answer to give. I will think more about this in detail and give an update here.

Generally speaking, the ref solution would work. I just personally don't like refs so much and I might have to introduce further useEffects for updating the values instead of just being able to pass a state. Although, yes it shouldn't make too much difference in the implementation.

I am personally working with a bigger software development team (product not involving MapKit JS) and we try to avoid refs wherever possible, as they can be a mess in maintenance and can be hard to debug but this again depends a lot on what they are used for and how. And yes, this rather applies to React than Javascript, though still it would be prettier for the code not having to use state and refs and just to rely on state. https://react.dev/learn/referencing-values-with-refs#best-practices-for-refs

Sign up for freeto 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.

2 participants

@nikischin@Nicolapps
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

Implement visible portion features - #50

Draft
nikischin wants to merge 3 commits into
Nicolapps:mainfrom
nikischin:feature/mapVisiblePortion
Draft

Implement visible portion features#50
nikischin wants to merge 3 commits into
Nicolapps:mainfrom
nikischin:feature/mapVisiblePortion

Conversation

@nikischin

@nikischinnikischin commented Oct 2, 2023

Copy link
Copy Markdown
Contributor

Implementing features for center, rotation, region and visibleMapRect. (See #49)

Still draft as I wasn't able to fully test the rotation yet, as either storybook or my browser is a bit buggy.

@Nicolapps however, could you possible do a quick review if the general implementation seems reasonable to you?

@Nicolapps

Copy link
Copy Markdown
Owner

Thank you for your draft PR! I have a few remarks about the proposed APIs:

  • A lot of the new properties are overlapping (region, camera distance, center, visible map rect). This can lead to situations where the same parameter is specified several times, and it’s not clear what’s supposed to happen in these cases. Maybe it could be clearer to limit the number of available properties, but provide a way to convert the values between each other. Or another option could be to edit the TypeScript types to make it clear which properties are not meant to be set at the same time (maybe with something like discriminated unions).
  • It’s not possible to do bidirectional bindings of these properties with the current APIs. For instance, it is possible to set the camera distance from the user’s state, but the user doesn’t know when the camera distance changes. They could listen to the region events, but they can’t access the new value of cameraDistance without using refs.
  • MapKit JS doesn’t always emit events with the latest region values when browsing the map. When implementing bidirectional binding for these properties, I’m concerned about issues caused by the mismatch between the latest state in MapKit and the one that the user received. It would be nice to create a few stories in Storybook to show how it works in practice.
  • For some of these properties, MapKit JS overrides the value provided with a more suitable value as soon as it is set. For instance, when setting region, MapKit JS will use a region that fits the map ratio, which is most often not the case for the provided value. This could lead to a difference between the value being set and the value being applied, which works well in an imperative API like MapKit’s but could cause issues in the declarative context of React.
  • Would you want to deprecate initialRegion, as your comment in the code seems to indicate? This could still be useful for cases where the user only wants to set the initial region and wants to let the user browse the map freely without having to manage bidirectional binding.
  • What happens if the user sets region to a constant? According to React’s semantic, we probably shouldn’t let the map move (or revert the change instantly after the user is done panning?). It could be confusing if the implementation lets the set value and the actual value drift. (This is an issue we already have with the current implementation of the marker position and selected attributes, but it’s probably less confusing in this case since the ways these values can be modified are more limited.)
  • regionUpdateAnimates seems like a nice API which can make a lot of sense if the implementation works as expected, even when there is some bidirectional binding being used.

In general, I’m not sure if we could make an API like this one work well with the limitations of React and MapKit JS. But I’d love to be proved wrong and see stories that show it in action!

Out of curiosity, do you have more details about your use cases that make controlling these properties through attributes instead of refs? Using refs worked well for my project where I let the user pan freely the map and sometimes need to animate a region change to a particular location, but I understand that it might be different in your case.

@nikischin

Copy link
Copy Markdown
ContributorAuthor

Hi @Nicolapps,

thank you so much for this valuable feedback! Those are all valid points which on most I haven't yet have a clear answer to give. I will think more about this in detail and give an update here.

Generally speaking, the ref solution would work. I just personally don't like refs so much and I might have to introduce further useEffects for updating the values instead of just being able to pass a state. Although, yes it shouldn't make too much difference in the implementation.

I am personally working with a bigger software development team (product not involving MapKit JS) and we try to avoid refs wherever possible, as they can be a mess in maintenance and can be hard to debug but this again depends a lot on what they are used for and how. And yes, this rather applies to React than Javascript, though still it would be prettier for the code not having to use state and refs and just to rely on state. https://react.dev/learn/referencing-values-with-refs#best-practices-for-refs

Sign up for freeto 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.

2 participants

@nikischin@Nicolapps
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Implement visible portion features - #50

Draft
nikischin wants to merge 3 commits into
Nicolapps:mainfrom
nikischin:feature/mapVisiblePortion
Draft

Implement visible portion features#50
nikischin wants to merge 3 commits into
Nicolapps:mainfrom
nikischin:feature/mapVisiblePortion

Conversation

@nikischin

@nikischinnikischin commented Oct 2, 2023

Copy link
Copy Markdown
Contributor

Implementing features for center, rotation, region and visibleMapRect. (See #49)

Still draft as I wasn't able to fully test the rotation yet, as either storybook or my browser is a bit buggy.

@Nicolapps however, could you possible do a quick review if the general implementation seems reasonable to you?

@Nicolapps

Copy link
Copy Markdown
Owner

Thank you for your draft PR! I have a few remarks about the proposed APIs:

  • A lot of the new properties are overlapping (region, camera distance, center, visible map rect). This can lead to situations where the same parameter is specified several times, and it’s not clear what’s supposed to happen in these cases. Maybe it could be clearer to limit the number of available properties, but provide a way to convert the values between each other. Or another option could be to edit the TypeScript types to make it clear which properties are not meant to be set at the same time (maybe with something like discriminated unions).
  • It’s not possible to do bidirectional bindings of these properties with the current APIs. For instance, it is possible to set the camera distance from the user’s state, but the user doesn’t know when the camera distance changes. They could listen to the region events, but they can’t access the new value of cameraDistance without using refs.
  • MapKit JS doesn’t always emit events with the latest region values when browsing the map. When implementing bidirectional binding for these properties, I’m concerned about issues caused by the mismatch between the latest state in MapKit and the one that the user received. It would be nice to create a few stories in Storybook to show how it works in practice.
  • For some of these properties, MapKit JS overrides the value provided with a more suitable value as soon as it is set. For instance, when setting region, MapKit JS will use a region that fits the map ratio, which is most often not the case for the provided value. This could lead to a difference between the value being set and the value being applied, which works well in an imperative API like MapKit’s but could cause issues in the declarative context of React.
  • Would you want to deprecate initialRegion, as your comment in the code seems to indicate? This could still be useful for cases where the user only wants to set the initial region and wants to let the user browse the map freely without having to manage bidirectional binding.
  • What happens if the user sets region to a constant? According to React’s semantic, we probably shouldn’t let the map move (or revert the change instantly after the user is done panning?). It could be confusing if the implementation lets the set value and the actual value drift. (This is an issue we already have with the current implementation of the marker position and selected attributes, but it’s probably less confusing in this case since the ways these values can be modified are more limited.)
  • regionUpdateAnimates seems like a nice API which can make a lot of sense if the implementation works as expected, even when there is some bidirectional binding being used.

In general, I’m not sure if we could make an API like this one work well with the limitations of React and MapKit JS. But I’d love to be proved wrong and see stories that show it in action!

Out of curiosity, do you have more details about your use cases that make controlling these properties through attributes instead of refs? Using refs worked well for my project where I let the user pan freely the map and sometimes need to animate a region change to a particular location, but I understand that it might be different in your case.

@nikischin

Copy link
Copy Markdown
ContributorAuthor

Hi @Nicolapps,

thank you so much for this valuable feedback! Those are all valid points which on most I haven't yet have a clear answer to give. I will think more about this in detail and give an update here.

Generally speaking, the ref solution would work. I just personally don't like refs so much and I might have to introduce further useEffects for updating the values instead of just being able to pass a state. Although, yes it shouldn't make too much difference in the implementation.

I am personally working with a bigger software development team (product not involving MapKit JS) and we try to avoid refs wherever possible, as they can be a mess in maintenance and can be hard to debug but this again depends a lot on what they are used for and how. And yes, this rather applies to React than Javascript, though still it would be prettier for the code not having to use state and refs and just to rely on state. https://react.dev/learn/referencing-values-with-refs#best-practices-for-refs

Sign up for freeto 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.

2 participants

@nikischin@Nicolapps
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Implement visible portion features - #50

Draft
nikischin wants to merge 3 commits into
Nicolapps:mainfrom
nikischin:feature/mapVisiblePortion
Draft

Implement visible portion features#50
nikischin wants to merge 3 commits into
Nicolapps:mainfrom
nikischin:feature/mapVisiblePortion

Conversation

@nikischin

@nikischinnikischin commented Oct 2, 2023

Copy link
Copy Markdown
Contributor

Implementing features for center, rotation, region and visibleMapRect. (See #49)

Still draft as I wasn't able to fully test the rotation yet, as either storybook or my browser is a bit buggy.

@Nicolapps however, could you possible do a quick review if the general implementation seems reasonable to you?

@Nicolapps

Copy link
Copy Markdown
Owner

Thank you for your draft PR! I have a few remarks about the proposed APIs:

  • A lot of the new properties are overlapping (region, camera distance, center, visible map rect). This can lead to situations where the same parameter is specified several times, and it’s not clear what’s supposed to happen in these cases. Maybe it could be clearer to limit the number of available properties, but provide a way to convert the values between each other. Or another option could be to edit the TypeScript types to make it clear which properties are not meant to be set at the same time (maybe with something like discriminated unions).
  • It’s not possible to do bidirectional bindings of these properties with the current APIs. For instance, it is possible to set the camera distance from the user’s state, but the user doesn’t know when the camera distance changes. They could listen to the region events, but they can’t access the new value of cameraDistance without using refs.
  • MapKit JS doesn’t always emit events with the latest region values when browsing the map. When implementing bidirectional binding for these properties, I’m concerned about issues caused by the mismatch between the latest state in MapKit and the one that the user received. It would be nice to create a few stories in Storybook to show how it works in practice.
  • For some of these properties, MapKit JS overrides the value provided with a more suitable value as soon as it is set. For instance, when setting region, MapKit JS will use a region that fits the map ratio, which is most often not the case for the provided value. This could lead to a difference between the value being set and the value being applied, which works well in an imperative API like MapKit’s but could cause issues in the declarative context of React.
  • Would you want to deprecate initialRegion, as your comment in the code seems to indicate? This could still be useful for cases where the user only wants to set the initial region and wants to let the user browse the map freely without having to manage bidirectional binding.
  • What happens if the user sets region to a constant? According to React’s semantic, we probably shouldn’t let the map move (or revert the change instantly after the user is done panning?). It could be confusing if the implementation lets the set value and the actual value drift. (This is an issue we already have with the current implementation of the marker position and selected attributes, but it’s probably less confusing in this case since the ways these values can be modified are more limited.)
  • regionUpdateAnimates seems like a nice API which can make a lot of sense if the implementation works as expected, even when there is some bidirectional binding being used.

In general, I’m not sure if we could make an API like this one work well with the limitations of React and MapKit JS. But I’d love to be proved wrong and see stories that show it in action!

Out of curiosity, do you have more details about your use cases that make controlling these properties through attributes instead of refs? Using refs worked well for my project where I let the user pan freely the map and sometimes need to animate a region change to a particular location, but I understand that it might be different in your case.

@nikischin

Copy link
Copy Markdown
ContributorAuthor

Hi @Nicolapps,

thank you so much for this valuable feedback! Those are all valid points which on most I haven't yet have a clear answer to give. I will think more about this in detail and give an update here.

Generally speaking, the ref solution would work. I just personally don't like refs so much and I might have to introduce further useEffects for updating the values instead of just being able to pass a state. Although, yes it shouldn't make too much difference in the implementation.

I am personally working with a bigger software development team (product not involving MapKit JS) and we try to avoid refs wherever possible, as they can be a mess in maintenance and can be hard to debug but this again depends a lot on what they are used for and how. And yes, this rather applies to React than Javascript, though still it would be prettier for the code not having to use state and refs and just to rely on state. https://react.dev/learn/referencing-values-with-refs#best-practices-for-refs

Sign up for freeto 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.

2 participants

@nikischin@Nicolapps
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

Implement visible portion features - #50

Draft
nikischin wants to merge 3 commits into
Nicolapps:mainfrom
nikischin:feature/mapVisiblePortion
Draft

Implement visible portion features#50
nikischin wants to merge 3 commits into
Nicolapps:mainfrom
nikischin:feature/mapVisiblePortion

Conversation

@nikischin

@nikischinnikischin commented Oct 2, 2023

Copy link
Copy Markdown
Contributor

Implementing features for center, rotation, region and visibleMapRect. (See #49)

Still draft as I wasn't able to fully test the rotation yet, as either storybook or my browser is a bit buggy.

@Nicolapps however, could you possible do a quick review if the general implementation seems reasonable to you?

@Nicolapps

Copy link
Copy Markdown
Owner

Thank you for your draft PR! I have a few remarks about the proposed APIs:

  • A lot of the new properties are overlapping (region, camera distance, center, visible map rect). This can lead to situations where the same parameter is specified several times, and it’s not clear what’s supposed to happen in these cases. Maybe it could be clearer to limit the number of available properties, but provide a way to convert the values between each other. Or another option could be to edit the TypeScript types to make it clear which properties are not meant to be set at the same time (maybe with something like discriminated unions).
  • It’s not possible to do bidirectional bindings of these properties with the current APIs. For instance, it is possible to set the camera distance from the user’s state, but the user doesn’t know when the camera distance changes. They could listen to the region events, but they can’t access the new value of cameraDistance without using refs.
  • MapKit JS doesn’t always emit events with the latest region values when browsing the map. When implementing bidirectional binding for these properties, I’m concerned about issues caused by the mismatch between the latest state in MapKit and the one that the user received. It would be nice to create a few stories in Storybook to show how it works in practice.
  • For some of these properties, MapKit JS overrides the value provided with a more suitable value as soon as it is set. For instance, when setting region, MapKit JS will use a region that fits the map ratio, which is most often not the case for the provided value. This could lead to a difference between the value being set and the value being applied, which works well in an imperative API like MapKit’s but could cause issues in the declarative context of React.
  • Would you want to deprecate initialRegion, as your comment in the code seems to indicate? This could still be useful for cases where the user only wants to set the initial region and wants to let the user browse the map freely without having to manage bidirectional binding.
  • What happens if the user sets region to a constant? According to React’s semantic, we probably shouldn’t let the map move (or revert the change instantly after the user is done panning?). It could be confusing if the implementation lets the set value and the actual value drift. (This is an issue we already have with the current implementation of the marker position and selected attributes, but it’s probably less confusing in this case since the ways these values can be modified are more limited.)
  • regionUpdateAnimates seems like a nice API which can make a lot of sense if the implementation works as expected, even when there is some bidirectional binding being used.

In general, I’m not sure if we could make an API like this one work well with the limitations of React and MapKit JS. But I’d love to be proved wrong and see stories that show it in action!

Out of curiosity, do you have more details about your use cases that make controlling these properties through attributes instead of refs? Using refs worked well for my project where I let the user pan freely the map and sometimes need to animate a region change to a particular location, but I understand that it might be different in your case.

@nikischin

Copy link
Copy Markdown
ContributorAuthor

Hi @Nicolapps,

thank you so much for this valuable feedback! Those are all valid points which on most I haven't yet have a clear answer to give. I will think more about this in detail and give an update here.

Generally speaking, the ref solution would work. I just personally don't like refs so much and I might have to introduce further useEffects for updating the values instead of just being able to pass a state. Although, yes it shouldn't make too much difference in the implementation.

I am personally working with a bigger software development team (product not involving MapKit JS) and we try to avoid refs wherever possible, as they can be a mess in maintenance and can be hard to debug but this again depends a lot on what they are used for and how. And yes, this rather applies to React than Javascript, though still it would be prettier for the code not having to use state and refs and just to rely on state. https://react.dev/learn/referencing-values-with-refs#best-practices-for-refs

Sign up for freeto 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.

2 participants

@nikischin@Nicolapps
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Implement visible portion features - #50

Draft
nikischin wants to merge 3 commits into
Nicolapps:mainfrom
nikischin:feature/mapVisiblePortion
Draft

Implement visible portion features#50
nikischin wants to merge 3 commits into
Nicolapps:mainfrom
nikischin:feature/mapVisiblePortion

Conversation

@nikischin

@nikischinnikischin commented Oct 2, 2023

Copy link
Copy Markdown
Contributor

Implementing features for center, rotation, region and visibleMapRect. (See #49)

Still draft as I wasn't able to fully test the rotation yet, as either storybook or my browser is a bit buggy.

@Nicolapps however, could you possible do a quick review if the general implementation seems reasonable to you?

@Nicolapps

Copy link
Copy Markdown
Owner

Thank you for your draft PR! I have a few remarks about the proposed APIs:

  • A lot of the new properties are overlapping (region, camera distance, center, visible map rect). This can lead to situations where the same parameter is specified several times, and it’s not clear what’s supposed to happen in these cases. Maybe it could be clearer to limit the number of available properties, but provide a way to convert the values between each other. Or another option could be to edit the TypeScript types to make it clear which properties are not meant to be set at the same time (maybe with something like discriminated unions).
  • It’s not possible to do bidirectional bindings of these properties with the current APIs. For instance, it is possible to set the camera distance from the user’s state, but the user doesn’t know when the camera distance changes. They could listen to the region events, but they can’t access the new value of cameraDistance without using refs.
  • MapKit JS doesn’t always emit events with the latest region values when browsing the map. When implementing bidirectional binding for these properties, I’m concerned about issues caused by the mismatch between the latest state in MapKit and the one that the user received. It would be nice to create a few stories in Storybook to show how it works in practice.
  • For some of these properties, MapKit JS overrides the value provided with a more suitable value as soon as it is set. For instance, when setting region, MapKit JS will use a region that fits the map ratio, which is most often not the case for the provided value. This could lead to a difference between the value being set and the value being applied, which works well in an imperative API like MapKit’s but could cause issues in the declarative context of React.
  • Would you want to deprecate initialRegion, as your comment in the code seems to indicate? This could still be useful for cases where the user only wants to set the initial region and wants to let the user browse the map freely without having to manage bidirectional binding.
  • What happens if the user sets region to a constant? According to React’s semantic, we probably shouldn’t let the map move (or revert the change instantly after the user is done panning?). It could be confusing if the implementation lets the set value and the actual value drift. (This is an issue we already have with the current implementation of the marker position and selected attributes, but it’s probably less confusing in this case since the ways these values can be modified are more limited.)
  • regionUpdateAnimates seems like a nice API which can make a lot of sense if the implementation works as expected, even when there is some bidirectional binding being used.

In general, I’m not sure if we could make an API like this one work well with the limitations of React and MapKit JS. But I’d love to be proved wrong and see stories that show it in action!

Out of curiosity, do you have more details about your use cases that make controlling these properties through attributes instead of refs? Using refs worked well for my project where I let the user pan freely the map and sometimes need to animate a region change to a particular location, but I understand that it might be different in your case.

@nikischin

Copy link
Copy Markdown
ContributorAuthor

Hi @Nicolapps,

thank you so much for this valuable feedback! Those are all valid points which on most I haven't yet have a clear answer to give. I will think more about this in detail and give an update here.

Generally speaking, the ref solution would work. I just personally don't like refs so much and I might have to introduce further useEffects for updating the values instead of just being able to pass a state. Although, yes it shouldn't make too much difference in the implementation.

I am personally working with a bigger software development team (product not involving MapKit JS) and we try to avoid refs wherever possible, as they can be a mess in maintenance and can be hard to debug but this again depends a lot on what they are used for and how. And yes, this rather applies to React than Javascript, though still it would be prettier for the code not having to use state and refs and just to rely on state. https://react.dev/learn/referencing-values-with-refs#best-practices-for-refs

Sign up for freeto 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.

2 participants

@nikischin@Nicolapps
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Implement visible portion features - #50

Draft
nikischin wants to merge 3 commits into
Nicolapps:mainfrom
nikischin:feature/mapVisiblePortion
Draft

Implement visible portion features#50
nikischin wants to merge 3 commits into
Nicolapps:mainfrom
nikischin:feature/mapVisiblePortion

Conversation

@nikischin

@nikischinnikischin commented Oct 2, 2023

Copy link
Copy Markdown
Contributor

Implementing features for center, rotation, region and visibleMapRect. (See #49)

Still draft as I wasn't able to fully test the rotation yet, as either storybook or my browser is a bit buggy.

@Nicolapps however, could you possible do a quick review if the general implementation seems reasonable to you?

@Nicolapps

Copy link
Copy Markdown
Owner

Thank you for your draft PR! I have a few remarks about the proposed APIs:

  • A lot of the new properties are overlapping (region, camera distance, center, visible map rect). This can lead to situations where the same parameter is specified several times, and it’s not clear what’s supposed to happen in these cases. Maybe it could be clearer to limit the number of available properties, but provide a way to convert the values between each other. Or another option could be to edit the TypeScript types to make it clear which properties are not meant to be set at the same time (maybe with something like discriminated unions).
  • It’s not possible to do bidirectional bindings of these properties with the current APIs. For instance, it is possible to set the camera distance from the user’s state, but the user doesn’t know when the camera distance changes. They could listen to the region events, but they can’t access the new value of cameraDistance without using refs.
  • MapKit JS doesn’t always emit events with the latest region values when browsing the map. When implementing bidirectional binding for these properties, I’m concerned about issues caused by the mismatch between the latest state in MapKit and the one that the user received. It would be nice to create a few stories in Storybook to show how it works in practice.
  • For some of these properties, MapKit JS overrides the value provided with a more suitable value as soon as it is set. For instance, when setting region, MapKit JS will use a region that fits the map ratio, which is most often not the case for the provided value. This could lead to a difference between the value being set and the value being applied, which works well in an imperative API like MapKit’s but could cause issues in the declarative context of React.
  • Would you want to deprecate initialRegion, as your comment in the code seems to indicate? This could still be useful for cases where the user only wants to set the initial region and wants to let the user browse the map freely without having to manage bidirectional binding.
  • What happens if the user sets region to a constant? According to React’s semantic, we probably shouldn’t let the map move (or revert the change instantly after the user is done panning?). It could be confusing if the implementation lets the set value and the actual value drift. (This is an issue we already have with the current implementation of the marker position and selected attributes, but it’s probably less confusing in this case since the ways these values can be modified are more limited.)
  • regionUpdateAnimates seems like a nice API which can make a lot of sense if the implementation works as expected, even when there is some bidirectional binding being used.

In general, I’m not sure if we could make an API like this one work well with the limitations of React and MapKit JS. But I’d love to be proved wrong and see stories that show it in action!

Out of curiosity, do you have more details about your use cases that make controlling these properties through attributes instead of refs? Using refs worked well for my project where I let the user pan freely the map and sometimes need to animate a region change to a particular location, but I understand that it might be different in your case.

@nikischin

Copy link
Copy Markdown
ContributorAuthor

Hi @Nicolapps,

thank you so much for this valuable feedback! Those are all valid points which on most I haven't yet have a clear answer to give. I will think more about this in detail and give an update here.

Generally speaking, the ref solution would work. I just personally don't like refs so much and I might have to introduce further useEffects for updating the values instead of just being able to pass a state. Although, yes it shouldn't make too much difference in the implementation.

I am personally working with a bigger software development team (product not involving MapKit JS) and we try to avoid refs wherever possible, as they can be a mess in maintenance and can be hard to debug but this again depends a lot on what they are used for and how. And yes, this rather applies to React than Javascript, though still it would be prettier for the code not having to use state and refs and just to rely on state. https://react.dev/learn/referencing-values-with-refs#best-practices-for-refs

Sign up for freeto 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.

2 participants

@nikischin@Nicolapps
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

Implement visible portion features - #50

Draft
nikischin wants to merge 3 commits into
Nicolapps:mainfrom
nikischin:feature/mapVisiblePortion
Draft

Implement visible portion features#50
nikischin wants to merge 3 commits into
Nicolapps:mainfrom
nikischin:feature/mapVisiblePortion

Conversation

@nikischin

@nikischinnikischin commented Oct 2, 2023

Copy link
Copy Markdown
Contributor

Implementing features for center, rotation, region and visibleMapRect. (See #49)

Still draft as I wasn't able to fully test the rotation yet, as either storybook or my browser is a bit buggy.

@Nicolapps however, could you possible do a quick review if the general implementation seems reasonable to you?

@Nicolapps

Copy link
Copy Markdown
Owner

Thank you for your draft PR! I have a few remarks about the proposed APIs:

  • A lot of the new properties are overlapping (region, camera distance, center, visible map rect). This can lead to situations where the same parameter is specified several times, and it’s not clear what’s supposed to happen in these cases. Maybe it could be clearer to limit the number of available properties, but provide a way to convert the values between each other. Or another option could be to edit the TypeScript types to make it clear which properties are not meant to be set at the same time (maybe with something like discriminated unions).
  • It’s not possible to do bidirectional bindings of these properties with the current APIs. For instance, it is possible to set the camera distance from the user’s state, but the user doesn’t know when the camera distance changes. They could listen to the region events, but they can’t access the new value of cameraDistance without using refs.
  • MapKit JS doesn’t always emit events with the latest region values when browsing the map. When implementing bidirectional binding for these properties, I’m concerned about issues caused by the mismatch between the latest state in MapKit and the one that the user received. It would be nice to create a few stories in Storybook to show how it works in practice.
  • For some of these properties, MapKit JS overrides the value provided with a more suitable value as soon as it is set. For instance, when setting region, MapKit JS will use a region that fits the map ratio, which is most often not the case for the provided value. This could lead to a difference between the value being set and the value being applied, which works well in an imperative API like MapKit’s but could cause issues in the declarative context of React.
  • Would you want to deprecate initialRegion, as your comment in the code seems to indicate? This could still be useful for cases where the user only wants to set the initial region and wants to let the user browse the map freely without having to manage bidirectional binding.
  • What happens if the user sets region to a constant? According to React’s semantic, we probably shouldn’t let the map move (or revert the change instantly after the user is done panning?). It could be confusing if the implementation lets the set value and the actual value drift. (This is an issue we already have with the current implementation of the marker position and selected attributes, but it’s probably less confusing in this case since the ways these values can be modified are more limited.)
  • regionUpdateAnimates seems like a nice API which can make a lot of sense if the implementation works as expected, even when there is some bidirectional binding being used.

In general, I’m not sure if we could make an API like this one work well with the limitations of React and MapKit JS. But I’d love to be proved wrong and see stories that show it in action!

Out of curiosity, do you have more details about your use cases that make controlling these properties through attributes instead of refs? Using refs worked well for my project where I let the user pan freely the map and sometimes need to animate a region change to a particular location, but I understand that it might be different in your case.

@nikischin

Copy link
Copy Markdown
ContributorAuthor

Hi @Nicolapps,

thank you so much for this valuable feedback! Those are all valid points which on most I haven't yet have a clear answer to give. I will think more about this in detail and give an update here.

Generally speaking, the ref solution would work. I just personally don't like refs so much and I might have to introduce further useEffects for updating the values instead of just being able to pass a state. Although, yes it shouldn't make too much difference in the implementation.

I am personally working with a bigger software development team (product not involving MapKit JS) and we try to avoid refs wherever possible, as they can be a mess in maintenance and can be hard to debug but this again depends a lot on what they are used for and how. And yes, this rather applies to React than Javascript, though still it would be prettier for the code not having to use state and refs and just to rely on state. https://react.dev/learn/referencing-values-with-refs#best-practices-for-refs

Sign up for freeto 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.

2 participants

@nikischin@Nicolapps