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.

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

watson wang | historical landmarks of Philadelphia #13

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

watsonvv
Copy link

@watsonvv watsonvv commented Oct 2, 2023

watson x. wang | historical landmarks of Philadelphia

Copy link
Collaborator

@SofiaFasullo SofiaFasullo left a comment

Choose a reason for hiding this comment

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

Great job! See my comments and the linter test on how to improve your project for a portfolio.

<p>Lorem ipsum dolor sit amet, consectetur adipiscing elit. Suspendisse molestie hendrerit quam, efficitur vestibulum justo vehicula a.</p>
<p>Nullam et metus eu eros facilisis commodo. Vivamus elementum nulla nisi, eget fermentum mauris placerat sed.</p>
<h1>History of Philadelphia</h1>
<img class = "img-eye" src="https://assets.visitphilly.com/wp-content/uploads/2018/06/philadelphia-world-heritage-city.png" width="200"></img>
Copy link
Collaborator

Choose a reason for hiding this comment

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

here (and everywhere you create an element of class "img-eye": lines 23, 40, 47, 54) you are specifying the width dimension of the image. you also did this in your css file, so the width = 200 code inside the element is unnecessary.

Suggested change
<img class = "img-eye" src="https://assets.visitphilly.com/wp-content/uploads/2018/06/philadelphia-world-heritage-city.png" width="200"></img>
<img class = "img-eye" src="https://upload.wikimedia.org/wikipedia/commons/thumb/b/b3/Philadelphia_city_hall.jpg/560px-Philadelphia_city_hall.jpg"></img>

alternately, you could define the dimensions/styling here in the html element and not in css - doubling up styling may be confusing, so I suggest choosing one - personally I think keeping styling separate in a css file makes more sense

Your css file styled this object of class "img-eye" as having width = 20px This did override the styling set in the index.html file image element, so be aware of that. Your final storymap showed an image with width = 20px, 200px. Another reason for that could have been my next point:

you did not set a unit - I think you would have needed to say width = 200px or width = 200vh or width = 200% if you are going to specify it here. You can find other styling dimension units documentation at these links:
https://developer.mozilla.org/en-US/docs/Learn/CSS/Building_blocks/Values_and_units#numbers_lengths_and_percentages
https://www.w3schools.com/cssref/css_units.php

@@ -68,3 +68,7 @@ main {
.slide-nav-select {
min-width: 0;
}

.img-eye{
width: 20px;
Copy link
Collaborator

Choose a reason for hiding this comment

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

this image is appearing very small on my browser - I would increase the size. Any easy way to do this would be width: 200px;
however, px are an absolute unit and it would take some back-and-forth resizing/checking to find the perfect px amount. If you want to snap the image to the width of the slide content, just put in a relative unit such as %

Suggested change
width: 20px;
width: 100%;

for more information check out the links in my comment on your html file!

Comment on lines +6 to 12
const baseTileLayer = L.tileLayer('https://tiles.stadiamaps.com/tiles/stamen_toner/{z}/{x}/{y}{r}.{ext}', {
minZoom: 0,
maxZoom: 20,
attribution: '&copy; <a href="https://www.stadiamaps.com/" target="_blank">Stadia Maps</a> &copy; <a href="https://www.stamen.com/" target="_blank">Stamen Design</a> &copy; <a href="https://openmaptiles.org/" target="_blank">OpenMapTiles</a> &copy; <a href="https://www.openstreetmap.org/copyright">OpenStreetMap</a> contributors',
ext: 'png'
});
baseTileLayer.addTo(map);
Copy link
Collaborator

Choose a reason for hiding this comment

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

the stamen_toner map looks great from afar, but when you zoom into certain areas it doesn't have a lot of detail - I would consider a tile styling that has more building footprints when zoomed in closer OR substitute your point data with polygon data so you get a better idea of what you are zooming into. Right now it looks like a large grey box for some of the locations zoomed in.

I suggest a the "moonlight" mapbox tile https://blog.mapbox.com/designing-moonlight-a-new-custom-map-d25afec2cc6e that looks similar to stamen toner but has more detail close up! You need to create a mapbox account and token to use it - go to https://docs.mapbox.com/help/tutorials/get-started-tokens-api/ to get started. Create your username and token and insert below in the code suggested. Then click use https://www.mapbox.com/studio/styles/add-style/mapbox/cj3kbeqzo00022smj7akz3o1e to add the moonlight style to your account, and copy style url provided and the Style ID is at the end of the URL. Let me know if you have any questions!

Suggested change
const baseTileLayer = L.tileLayer('https://tiles.stadiamaps.com/tiles/stamen_toner/{z}/{x}/{y}{r}.{ext}', {
minZoom: 0,
maxZoom: 20,
attribution: '&copy; <a href="https://www.stadiamaps.com/" target="_blank">Stadia Maps</a> &copy; <a href="https://www.stamen.com/" target="_blank">Stamen Design</a> &copy; <a href="https://openmaptiles.org/" target="_blank">OpenMapTiles</a> &copy; <a href="https://www.openstreetmap.org/copyright">OpenStreetMap</a> contributors',
ext: 'png'
});
baseTileLayer.addTo(map);
const mapboxToken = ""; //insert your mapbox token here
const mapboxAccount = ""; //insert your account username here - if you are using a generic style like Light you can just put 'mapbox'
const mapboxStyleID = ""; //insert special code just for your account if it is a special or custom style, if it is a generic style like Light insert 'light-v10'
const baseTileLayer = L.tileLayer(`https://api.mapbox.com/styles/v1/${mapboxAccount}/${mapboxStyleID}/tiles/256/{z}/{x}/{y}@2x?access_token=${mapboxToken}`, {
minZoom: 0,
maxZoom: 20,
attribution: '© <a href="https://www.mapbox.com/about/maps/">Mapbox</a> © <a href="http://www.openstreetmap.org/copyright">OpenStreetMap</a>',
});
baseTileLayer.addTo(map);

<h2>Franklin Square</h2>
<p>Fun and games in one of William Penn’s original public squares...</p>
<img class = "img-eye" src="https://www.visitphilly.com/wp-content/uploads/2022/03/Daytime-Franklin-Square-Fountain-J-Fusco-for-vp-1200x900px-1044x781.jpg" width="200"></img>
<p>With a colorful fountain show and activities like mini-golf and carousel rides, Franklin Square offers a carefree respite from the sometimes-serious business of historic sightseeing, but you better believe the historic elements are still there. This delightful patch of green space that lies north of Independence Hall was one of five public squares laid out by William Penn in his original vision for Philadelphia. From Memorial Day to Labor Day, the park is home to a Once Upon a Nation storytelling bench, featuring short stories about the area from informed historical interpreters.</p>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I see that a lot of your text content is sourced from visitphilly.com. please cite all your sources or it is considered plagiarism and is not appropriate to display on your portfolio. Example citation:

Suggested change
<p>With a colorful fountain show and activities like mini-golf and carousel rides, Franklin Square offers a carefree respite from the sometimes-serious business of historic sightseeing, but you better believe the historic elements are still there. This delightful patch of green space that lies north of Independence Hall was one of five public squares laid out by William Penn in his original vision for Philadelphia. From Memorial Day to Labor Day, the park is home to a Once Upon a Nation storytelling bench, featuring short stories about the area from informed historical interpreters.</p>
<p>With a colorful fountain show and activities like mini-golf and carousel rides, Franklin Square offers a carefree respite from the sometimes-serious business of historic sightseeing, but you better believe the historic elements are still there. This delightful patch of green space that lies north of Independence Hall was one of five public squares laid out by William Penn in his original vision for Philadelphia. From Memorial Day to Labor Day, the park is home to a Once Upon a Nation storytelling bench, featuring short stories about the area from informed historical interpreters.
-Source: https://www.visitphilly.com/articles/philadelphia/must-see-historic-attractions-in-historic-philadelphia/
</p>

<p>Lorem ipsum dolor sit amet, consectetur adipiscing elit. Suspendisse molestie hendrerit quam, efficitur vestibulum justo vehicula a.</p>
<p>Nullam et metus eu eros facilisis commodo. Vivamus elementum nulla nisi, eget fermentum mauris placerat sed.</p>
<h1>History of Philadelphia</h1>
<img class = "img-eye" src="https://assets.visitphilly.com/wp-content/uploads/2018/06/philadelphia-world-heritage-city.png" width="200"></img>
Copy link
Collaborator

Choose a reason for hiding this comment

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

here (and everywhere you create an element of class "img-eye": lines 23, 40, 47, 54), when running AxeDevTools in my developer window I see that you have an accessibility issue. It suggests to add an alt argument to add alternative text that could be read out load if someone could not see the image

Suggested change
<img class = "img-eye" src="https://assets.visitphilly.com/wp-content/uploads/2018/06/philadelphia-world-heritage-city.png" width="200"></img>
<img class = "img-eye" alt = "A seal of Philadelphia showing an image of city hall and reading Philadelphia world heritage city" src="https://assets.visitphilly.com/wp-content/uploads/2018/06/philadelphia-world-heritage-city.png""></img>

for more information, see https://dequeuniversity.com/rules/axe/4.8/image-alt?application=AxeChrome

@watsonvv watsonvv changed the title watson x. wang | historical landmarks of Philadelphia watson wang | historical landmarks of Philadelphia Dec 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants