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

fix: estates within districts #662

Merged
merged 2 commits into from Nov 14, 2018

Conversation

Projects
None yet
3 participants
@cazala
Copy link
Member

cazala commented Nov 9, 2018

This PR fixes the map rendered, the click handler and the popup for estates that are deployed within districts (this includes plazas and roads)

How to test

Here are some queries to insert estates inside a plaza, a road and a district:

Insert an estate in Genesis Plaza (-4, 8)

INSERT INTO estates (id, owner, data, last_transferred_at, created_at, updated_at, tx_hash, token_id) VALUES ('9000', '0xfa5ad346a73fcd3defa42c694a710fe3c3dc6405', '{"ipns": "", "name": "TEST", "version": 0, "description": "", "parcels": ""}', 1541071498000, now(), now(), '0xdeadbeef', '9000');
UPDATE parcels SET estate_id = '9000' WHERE id IN ('-4,8', '-4,7');
UPDATE estates E SET data = jsonb_set(
  data,
  '{parcels}',
  (SELECT json_agg(json_build_object('x', P.x, 'y', P.y)) FROM parcels P WHERE P.estate_id = E.id)::jsonb
) WHERE E.id = '9000';

Insert an estate in a road (-10, 8)

INSERT INTO estates (id, owner, data, last_transferred_at, created_at, updated_at, tx_hash, token_id) VALUES ('9001', '0xfa5ad346a73fcd3defa42c694a710fe3c3dc6405', '{"ipns": "", "name": "TEST", "version": 0, "description": "", "parcels": ""}', 1541071498000, now(), now(), '0xfaceb00k', '9001');
UPDATE parcels SET estate_id = '9001' WHERE id IN ('-10,8', '-10,7');
UPDATE estates E SET data = jsonb_set(
  data,
  '{parcels}',
  (SELECT json_agg(json_build_object('x', P.x, 'y', P.y)) FROM parcels P WHERE P.estate_id = E.id)::jsonb
) WHERE E.id = '9001';

Insert an estate in Bittrex Tomorrow district (-57, 8)

INSERT INTO estates (id, owner, data, last_transferred_at, created_at, updated_at, tx_hash, token_id) VALUES ('9002', '0xfa5ad346a73fcd3defa42c694a710fe3c3dc6405', '{"ipns": "", "name": "TEST", "version": 0, "description": "", "parcels": ""}', 1541071498000, now(), now(), '0xcafebabe', '9002');
UPDATE parcels SET estate_id = '9002' WHERE id IN ('-57,8', '-57,7');
UPDATE estates E SET data = jsonb_set(
  data,
  '{parcels}',
  (SELECT json_agg(json_build_object('x', P.x, 'y', P.y)) FROM parcels P WHERE P.estate_id = E.id)::jsonb
) WHERE E.id = '9002';

The map should behave unaffected by these estates (rendering/clicking/hovering)

@cazala cazala requested review from abarmat and NicoSantangelo Nov 9, 2018

@cazala cazala force-pushed the fix/estates-within-districts branch from 4ad1913 to 702de49 Nov 9, 2018

@cazala cazala force-pushed the fix/estates-within-districts branch from 702de49 to 1687ea0 Nov 9, 2018

@abarmat

This comment has been minimized.

Copy link
Member

abarmat commented Nov 10, 2018

Map is fixed with this PR 👍

Before
before a

After
after a

Before
before b

After
after b

However I can still visit the detail pages and see the (hidden) Estates:

  • /estates/9000/detail
  • /estates/9001/detail
  • /estates/9002/detail

detail

@cazala

This comment has been minimized.

Copy link
Member

cazala commented Nov 11, 2018

@abarmat good catch. There was no current behaviour for "not found" assets (it just shows the spinner forever, see prod: https://market.decentraland.org/parcels/99999/99999/detail)

So Ia added a feat to show a "Not Found" page when an asset doesn't exist:

screen shot 2018-11-11 at 3 51 30 pm

You can test it on parcels and estates using urls like /parcels/999/999/detail or /estates/1337/detail

And I also added a rule to make any Estate that contains at least 1 parcel with a district_id to render that screen as well, so you should see it for these:

However I can still visit the detail pages and see the (hidden) Estates:

/estates/9000/detail
/estates/9001/detail
/estates/9002/detail

thanks for the review ^.^

@abarmat
Copy link
Member

abarmat left a comment

💯

@NicoSantangelo
Copy link
Member

NicoSantangelo left a comment

Great work

? TYPES.taken
: TYPES.unowned
}
}

This comment has been minimized.

@NicoSantangelo

NicoSantangelo Nov 14, 2018

Member

Maybe this file should be tile.js ? Viewport and Bounds are classes

This comment has been minimized.

@cazala

cazala Nov 14, 2018

Member

I changed it (https://github.com/decentraland/marketplace/pull/662/files#diff-3bba0f257a66b9b1031f5b67f3165da0R2) but git doesn't recognize the change on the file name :/

This comment has been minimized.

@cazala

cazala Nov 14, 2018

Member

nvm 😅

@@ -1,5 +1,5 @@
import { Selection, Parcel } from '.'
import { COLORS, getColor, getMapAsset } from '../../asset'
import { COLORS, getColor } from '../'

This comment has been minimized.

@NicoSantangelo

NicoSantangelo Nov 14, 2018

Member

Question
I'm under the impression that doing this:

import { Parcel } from './Parcel'
import { Selection } from './Selection'
import { COLORS, getColor } from '../Tile'

instead of

import { Selection, Parcel } from '.'
import { COLORS, getColor } from '../'

it's better to improve readability. For example, to write this very comment I had to check which files contained what.
I know the rest of shared/map uses this form so I'm not asking so we can change it, but rather to see what you think for later.

Same for https://github.com/decentraland/marketplace/pull/662/files#diff-24a4224e80ed9fc569fb73f3ead88f6b

asset,
x,
y
})

This comment has been minimized.

@NicoSantangelo

NicoSantangelo Nov 14, 2018

Member

onClick({type, asset, x, y }) 🙏

function* handleFetchEstateSuccess({ estate }) {
const areLoaded = yield select(state => areParcelsLoaded(state, estate))
if (!areLoaded) {
const parcels = yield select(state => state.parcels.data)

This comment has been minimized.

@NicoSantangelo

NicoSantangelo Nov 14, 2018

Member

maybe use getData from module/parcels/selectors here?

@cazala cazala force-pushed the fix/estates-within-districts branch from 45b0773 to e7d56cd Nov 14, 2018

@cazala cazala force-pushed the fix/estates-within-districts branch from e7d56cd to 78e39dc Nov 14, 2018

@NicoSantangelo NicoSantangelo merged commit 0a1b122 into master Nov 14, 2018

1 check passed

ci/circleci: build Your tests passed on CircleCI!
Details

@NicoSantangelo NicoSantangelo deleted the fix/estates-within-districts branch Nov 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment