Feature/autopolygon leasehold icon #234
Conversation
Hazz223
requested review from
martinmcneela,
ChrissyABrown and
iza-pik
as code owners
| const LeaseholdAutoPolygonIcon: React.FC<Props> = ({ size = 'large', fill = DEFAULT_ICON_COLOR, ...props }) => ( | ||
| <IconWrapper size={size}> | ||
| <LeaseholdAutoPolygon fill={fill} {...props} /> | ||
| </IconWrapper> | ||
| ); | ||
| export { LeaseholdAutoPolygonIcon, autoPolygonUrl as leaseholdAutoPolygonUrl }; |
There was a problem hiding this comment.
I think you can just rename AutoPolygonIcon rather than create a duplicate with a different name.
There was a problem hiding this comment.
You'll need to add it to the stories file too so it shows up in Stroybook, which I'm happy to help with if you need it
There was a problem hiding this comment.
I'd prefer not to change the name, as it would introduce a breaking change to the code base. This slow API migration means we can remove the old AutoPolygonIcon component once we're happy.
There was a problem hiding this comment.
So that icon isn't used anywhere else (only in maps), but that's fine if we want to approach it like that, can you add a note to remove the duplicate though as we tend to forget about these things.
There was a problem hiding this comment.
The app code can be changed to use the new name when updating the wombat version in Maps, so it shouldn’t be too risky in any case
|
This svg is 25 24 which is a little odd as mostly they are 24 24 which keeps them a square |
I've added two new icons; one for
LeaseholdAutoPolygonIcon, one forFreeholdAutoPolygonIcon. This leaves the currentAutoPolygonIconicon alone.It should be noted that
LeaseholdAutoPolygonIconis the same asAutoPolygonIcon- this will allow us to migrate away fromAutoPolygonIconwhile maintaining backwards compatibility.