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.

Sign up for GitHub

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

Improve "place in" box on dev place pages #4774

Merged
merged 12 commits into from
Dec 9, 2024
Merged

Improve "place in" box on dev place pages #4774

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
a6ae975
add continent to country mapping
juliawu Dec 5, 2024
1b809c8
add toggle to related places
juliawu Dec 6, 2024
aa15fd3
Merge branch 'master' of /~https://github.com/datacommonsorg/website in…
juliawu Dec 6, 2024
cc21801
simplify truncated list logic
juliawu Dec 6, 2024
8735c5f
lint
juliawu Dec 6, 2024
7155ad0
Merge branch 'master' into new-places-in
juliawu Dec 9, 2024
c3fd13a
add number to show more
juliawu Dec 9, 2024
ed33faa
remove place search and add dcid link
juliawu Dec 9, 2024
0a22e9a
Merge branch 'master' of /~https://github.com/datacommonsorg/website in…
juliawu Dec 9, 2024
e33ccea
remove place search test
juliawu Dec 9, 2024
d42f8a5
Merge branch 'master' into new-places-in
juliawu Dec 9, 2024
3334241
Merge branch 'master' into new-places-in
juliawu Dec 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions server/routes/dev_place/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -313,6 +313,7 @@ def chart_config_to_overview_charts(chart_config, child_place_type: str):
# Maps each parent place type to a list of valid child place types.
# This hierarchy defines how places are related in terms of containment.
PLACE_TYPES_TO_CHILD_PLACE_TYPES = {
"Continent": ["Country"],
"GeoRegion": ["Country", "City"],
"Country": [
"State", "EurostatNUTS1", "EurostatNUTS2", "AdministrativeArea1"
Expand Down
12 changes: 12 additions & 0 deletions static/css/place/dev_place_page.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,4 +158,16 @@ main {
font-weight: 400;
line-height: 28px;
}

.show-more-toggle {
align-items: center;
color: var(--link-color);
cursor: pointer;
display: flex;
gap: 2px;
font-size: 14px;
font-weight: 500;
padding-left: 8px;
padding-top: 16px;
}
}
36 changes: 29 additions & 7 deletions static/js/place/dev_place_main.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -395,24 +395,46 @@ const RelatedPlaces = (props: {
place: NamedTypedPlace;
childPlaces: NamedTypedPlace[];
}) => {
const [isCollapsed, setIsCollapsed] = useState(true);
const { place, childPlaces } = props;
if (!childPlaces || childPlaces.length === 0) {
return null;
}
const NUM_PLACES = 15;
Copy link
Contributor

Choose a reason for hiding this comment

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

curious - how did we choose 15?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Admittedly a bit arbitrarily -- this looks good to my eyes for now. My thought is to use NUM_PLACES = 15 to unblock experimental release before the code freeze, and we can decide on a better solution after.

Copy link
Contributor

Choose a reason for hiding this comment

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

After thinking about this more, i think a better experience might be to make this responsive and to show more items when the page is larger, and fewer for smaller screen sizes. Wdyt @miss-o-soup ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, I'll discuss with @miss-o-soup and add responsive behavior in a follow-up PR, I don't think this needs to block release.

const showToggle = childPlaces.length > NUM_PLACES;
const toggleShowMore = () => {
setIsCollapsed(!isCollapsed);
};

return (
<div className="related-places">
<div className="related-places-callout">Places in {place.name}</div>
<div className="item-list-container">
<div className="item-list-inner">
{childPlaces.map((place) => (
<div key={place.dcid} className="item-list-item">
<a className="item-list-text" href={`/dev-place/${place.dcid}`}>
{place.name}
</a>
</div>
))}
{childPlaces
Copy link
Contributor

Choose a reason for hiding this comment

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

I might argue this is starting to add a lot of logic within the render block.
could we maybe store the slice in a different variable called trimmedChildPlaces and then just toggle between trimmedChildPlaces and allChildPlaces?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call out. Stored the slice in a different variable and now just toggle between the lists which looks much cleaner now.

.slice(
0,
showToggle && isCollapsed ? NUM_PLACES : childPlaces.length
)
.map((place) => (
<div key={place.dcid} className="item-list-item">
<a className="item-list-text" href={`/dev-place/${place.dcid}`}>
{place.name}
</a>
</div>
))}
</div>
</div>
{showToggle && (
<div className="show-more-toggle" onClick={toggleShowMore}>
<span className="show-more-toggle-text">
{isCollapsed ? "Show more" : "Show less"}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add the count for show more? e.g., "Show 25 more"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great idea, done! Also updated screenshots in the PR description.

</span>
<span className="material-icons-outlined">
{isCollapsed ? "expand_more" : "expand_less"}
</span>
</div>
)}
</div>
);
};
Expand Down
Loading