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

Address QA review of stage/v0.6.0 #451

Merged
merged 2 commits into from
Feb 3, 2023
Merged

Address QA review of stage/v0.6.0 #451

merged 2 commits into from
Feb 3, 2023

Conversation

danielfdsilva
Copy link
Collaborator

Problems covered by this PR

Report: https://www.dropbox.com/scl/fi/kjroygitvhc8f2kh2nhuw/Veda-Dashboard-QA-Stage-deploy-v0.6.0.paper?dl=0&rlkey=ls3fz1idxvq03hlzewfnblnnz

Broken polygons in analysis page

We were using the double pipe || as a way to separate polygons in the url, however a polyline has naturally occurring double pipes, so when the polyline of a complex polgon had them, it was being broken into multiple polygons.
Replaced the separator with a semicolon ; which is not present in the polyline.

Overlay z-index

Updated the z-index of the dropdown and created a list of z-indices in the theme to simplify usage. Also converted the theme file to typescript which required updating the typings of themeVal

@netlify
Copy link

netlify bot commented Feb 3, 2023

Deploy Preview for veda-ui ready!

Name Link
🔨 Latest commit 1e2830d
🔍 Latest deploy log https://app.netlify.com/sites/veda-ui/deploys/63dcea125e635e00080b5ec9
😎 Deploy Preview https://deploy-preview-451--veda-ui.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

Copy link
Contributor

@nerik nerik left a comment

Choose a reason for hiding this comment

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

Good catch about the polyline encoding.

Replaced the separator with a semicolon ; which is not present in the polyline.

Are you 100% sure of that? I couldn't find any info on what characters are included or not.
Additionally, I'm not sure semicolons are safe to use in URLs, see

Maybe a safer bet would be to use a longer string?
I'm also wondering if at this point we should provide retrocompatibility to URLs formed in a previous version (probably not; although something to keep in mind)

@danielfdsilva
Copy link
Collaborator Author

danielfdsilva commented Feb 3, 2023

@nerik According to the polyline encoding they add 63 before converting to ASCII. The ASCII code for the semicolon is below 63, so it won't appear.
Since the values get URI encoded I'm not too worried about using it in the url.

Maybe a safer bet would be to use a longer string?

What do you mean?

Retro-compatibility is a good call. Agree that it is probably not needed at this point, but something we have to keep in mind!

@nerik
Copy link
Contributor

nerik commented Feb 3, 2023

Oh ok, perfect then.

@nerik nerik self-requested a review February 3, 2023 15:10
@danielfdsilva danielfdsilva merged commit 6ce01ba into main Feb 3, 2023
@danielfdsilva danielfdsilva deleted the cr-2023.02 branch February 3, 2023 15:13
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.

2 participants