[Uptime] Fix/location map hide layer view control#53568
[Uptime] Fix/location map hide layer view control#53568shahzad31 merged 11 commits intoelastic:masterfrom
Conversation
shahzad31
added
release_note:skip
|
Pinging @elastic/uptime (Team:uptime) |
💚 Build SucceededTo update your PR or re-run it, just comment with: |
justinkambic
left a comment
justinkambic
left a comment
There was a problem hiding this comment.
Had a few questions/refinements, but the overall changes looks good. Keeps us going in the right direction, so excited to see this page getting more and more ❤️ over time!
| import { UptimeSettingsContext } from '../../../contexts'; | ||
|
|
||
| const TextStyle = styled.div` | ||
| text-transform: uppercase; |
There was a problem hiding this comment.
Do we want this? The screenshot in the issue has a location (I think it was Japan) like Japan.
On my screen my locations render like this:
I found it kind of unsettling when I saw them in caps, as I've specified them like Fairbanks and Us-east-2.
There was a problem hiding this comment.
i agree, i also got chance to discuss with andrew, so have remote the caps.
| `; | ||
|
|
||
| interface Props { | ||
| monitorLocations: any; |
There was a problem hiding this comment.
Can we declare this prop as monitorLocations: any[] (or just name it locations) and pass monitorLocations?.locations ?? [] from the parent? That way we can avoid the nesting below and we won't be calling forEach on an any type.
| }); | ||
| } | ||
| return ( | ||
| <TagContainer> |
There was a problem hiding this comment.
How does this look when there are no locations specified? Do we need an alternate render (or null), or is it ok with no children returned by the map calls?
There was a problem hiding this comment.
i think it's fine, it's not obstructive.
|
@elasticmachine merge upstream |
|
@justinkambic i took care of your comments, you can re-review this now. |
|
@elasticmachine merge upstream |
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
justinkambic
left a comment
justinkambic
left a comment
There was a problem hiding this comment.
LGTM - one code change recommended
| export const getLayerList = ( | ||
| upPoints: LocationPoint[], | ||
| downPoints: LocationPoint[], | ||
| { gray, danger }: { gray: string; danger: string } |
There was a problem hiding this comment.
There's actually an UptimeAppColors type that we could use here instead of declaring the interface inline.
There was a problem hiding this comment.
I have taken care of this code change in my other related PR to this file. So i will merge this.
* hide layer control and added loc tags * update test * remove unused comment * remove capitalization * style fix * update types Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
4 participants
Summary
Fixes: #53563
This PR will hide view/layer control from location map in details panel.
After:
Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.For maintainers