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

Allow for exception page titles #139

Closed
wants to merge 1 commit into from
Closed

Allow for exception page titles #139

wants to merge 1 commit into from

Conversation

dhcole
Copy link

@dhcole dhcole commented Feb 27, 2015

Currently exceptions override page urls or domains but not page titles. This adds the option to set an exception object that specifies both a page and page_title. It's implemented on the weather map to avoid matching items:

Before

screen shot 2015-02-27 at 12 21 26 pm

After

screen shot 2015-02-27 at 12 25 16 pm

Since these pages don't have a unique title, I borrowed from the link's title.

screen shot 2015-02-27 at 12 02 27 pm

Also adds missing title attribute to links for the realtime list.

@konklone
Copy link
Contributor

I appreciate what this is for, but my immediate reaction is a 👎. We don't want to be in the business of manually maintaining how the Top 20 listings look. We've made a few URL exceptions for mainstays of the Top 20, but I'd also be against adding many more URL exceptions there too. And the only reason I think we're comfortable doing even those is that URL structure is much harder to change than a <title> tag. So I'd prefer to channel pressure to clean this up to the agency in question.

@cew821
Copy link
Contributor

cew821 commented Mar 1, 2015

While I agree with @konklone that it would be nice to not have to clean these up, I also think that it will provide a nicer user experience to accept this edit and do a bit of manual labor on sites that are persistently in the top 20. I find both arguments compelling, so I don't think this is clear cut and would like others to weigh in. cc @leahbannon @gbinal

@gbinal
Copy link
Member

gbinal commented Mar 2, 2015

I think the move is to do this with a light touch, for a few that - like people have said - are reliably in the top 20. We don't take ensuring this for the top 20 and we do use channel pressure like @konklone says but we allow ourselves to ameliorate this some.

@konklone
Copy link
Contributor

konklone commented Mar 2, 2015

In that case, we don't have to do anything. Nothing is reliably in the Top 20 that suffers from this problem. :) Let's move on, and deal it with reactively if it becomes an issue.

@konklone konklone closed this Mar 2, 2015
@cew821 cew821 reopened this Mar 18, 2015
@cew821
Copy link
Contributor

cew821 commented Mar 18, 2015

I think we should reconsider this PR, for the specific case of the National Weather Service. This is consistently on the Top 20, and very confusing since both links go to the same place.

@konklone
Copy link
Contributor

@cew821 Can you suggest what the different titles should be for the two weather domains?

We're currently mapping forecast.weather.gov/mapclick.php to www.weather.gov, so the real comparison is between whatever people are doing at forecast.weather.gov/mapclick.php and whatever people do on weather.gov. I don't know how to describe the difference.

@cew821
Copy link
Contributor

cew821 commented Mar 18, 2015

Users are viewing a forecast for a specific region, i.e. http://forecast.weather.gov/MapClick.php?x=165&y=52&site=pbz&zmx=1&zmy=1&map_x=165&map_y=52#.VQmg59X6ePU

So how about "National Weather Service - Forecasts by Region"

@cew821
Copy link
Contributor

cew821 commented Mar 18, 2015

We could also map the URL to "forecast.weather.gov" which I realize just redirects to Weather.gov, but at least it would be a different behavior than the first link.

@konklone
Copy link
Contributor

👍 on both counts.

@konklone konklone added this to the Public Launch milestone Mar 18, 2015
@konklone
Copy link
Contributor

Fixed in f72b9ed.

@konklone konklone closed this Mar 19, 2015
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.

4 participants