-
Notifications
You must be signed in to change notification settings - Fork 5
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
He updates #654
He updates #654
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@@ -70,6 +70,7 @@ $chart-margin: 14px; | |||
.tickValue { | |||
@extend %annotation; | |||
color: $alto; | |||
font-size: 18px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you use a variable for this? It's true that we dont have any variable for font-size: 18px. But maybe create a new one on src/styles/settings.scss
@@ -87,7 +88,7 @@ | |||
.legendTitle { | |||
color: $grey-text; | |||
font-weight: 300; | |||
font-size: 13px; | |||
font-size: 17px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, is to have all font-sizes tracked
@@ -189,7 +189,7 @@ | |||
} | |||
|
|||
input:checked ~ .checkbox { | |||
background-color: #2196F3; | |||
background-color: #2196f3; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be also great to have this as a variable
return ( | ||
<div | ||
className={cx(styles.sidebarCardContainer, className, { | ||
[styles.onboardingOverlay]: | ||
onboardingType === 'priority-places' && onboardingStep === 2, | ||
})} | ||
> | ||
<button |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know why this button is needed? The Analysis radio button was supposed to have always an option selected. Also, maybe if its closer to the options would be easier to the user to understand its functionality
@Bluesmile82 i made changes based on your requests. The Clear Filters button was a request by Alex to allow the visitor to remove all layers applied. I did move the button to underneath the radio options to make it easier for the user to understand |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the changes! Please check the comments, the logic toggling the layers is not working as expected. I think it might have to do with the former selected slug not being the one we expect. Also we have some problems when they are several layers selected due to the selection. Maybe the logic has to change at a higher level.
@@ -63,8 +63,8 @@ | |||
.yAxisIndicator { | |||
@extend %bodyText; | |||
color: $alto; | |||
font-size: 18px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you use the variable also here?
@@ -77,8 +77,8 @@ | |||
position: relative; | |||
bottom: 50%; | |||
left: 2px; | |||
font-size: 18px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And here
} | ||
|
||
.legendItem { | ||
align-items: center; | ||
color: $white; | ||
display: flex; | ||
margin: 0; | ||
font-size: 18px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And the last missing
layerId: newSelectedOption, | ||
category: newLayerCategory, | ||
}); | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The clear and the selection is not working correctly here in relation to the layers. Two examples:
- Go to the analyze areas
- Click on the clear button. The radio is deselected ✔️
- Click on the Places for half earth future. The future layer is not selected ✖️
- Click on the Administrative Boundaries. Now the future layer is selected ✖️
- Go to the analyze areas
- Click on the Places for half earth future. The future layer is selected ✔️
- Click on Protected areas. The protected layers are selected ✔️
- Click on the clear button. The places for half earth future layer is still selected ✖️
className={styles.clearFilters} | ||
onClick={clearFilters} | ||
> | ||
clear filters |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We think that 'Clear selection' is more accurate as a text. As these are more options for the user to interact than filters. Although they do filter and select layers
@Bluesmile82 for now i removed the Clear selections button seems I need more time to get the functionality correct and wanted to get the requested style changes in while I still work on the button. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, please comment or bring back the code before merging
Update styling for Analyze area charts
Description
Made labels bigger and easier to read by increasing the font size. Also added the ability to clear all filters applied. Also added color.shamrock to exported colors so the Chart would display the Africa legend color
Designs
Link to the related design prototypes (if relevant)
Testing instructions
Explain how the reviewer should test this PR (which URL should be joined, which steps should be performed...)
Feature relevant tickets
Link to the related task manager tickets