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

IFSC Website #32

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

IFSC Website #32

wants to merge 11 commits into from

Conversation

captn3m0
Copy link
Contributor

@captn3m0 captn3m0 commented Jan 21, 2019

Testing instructions:

docker build -t ifsc-website . && echo "RUNNING" && docker run -it  --publish 3000:3000 ifsc-website

Then open one of these in the browser:

TODO:

  • Improve design
  • /banks/:code doesn't seem to
  • Remove metrics
  • Check review comments.

@captn3m0 captn3m0 self-assigned this Feb 27, 2019
Copy link
Contributor

@patlola patlola left a comment

Choose a reason for hiding this comment

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

will review completely after discussing offline once.

</tr>
<tr>
<th>Contact</th>
<td><%= data['CONTACT']%></td>
Copy link
Contributor

Choose a reason for hiding this comment

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

some of the data is null and it's not sanitized properly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, design is WIP for now.

@@ -0,0 +1 @@
Sitemap: https://ifsc.stage.razorpay.in/sitemap.xml.gz
Copy link
Contributor

Choose a reason for hiding this comment

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

put the prod url. until we submit the sitemap, search engines will not crawl i guess

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool.

d.delete_if { |key| %w[BANK IFSC].include? key }
redis.hmset ifsc, *d
# CHANGE TO .com before merging
SitemapGenerator::Sitemap.default_host = 'https://ifsc.stage.razorpay.in'
Copy link
Contributor

Choose a reason for hiding this comment

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

get it from config may be?

if data['BANK'] && data['BRANCH']
url = "/#{data['BANK'].to_s.to_slug}/#{data['BRANCH'].to_s.to_slug}/#{ifsc}"
end
if url
Copy link
Contributor

Choose a reason for hiding this comment

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

merge both if's

</tr>
<tr>
<th>Address</th>
<td><%= data['ADDRESS']%></td>
Copy link
Contributor

Choose a reason for hiding this comment

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

also, add https://schema.org/ context schema so that search engines recognize the data easily

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

Successfully merging this pull request may close these issues.

2 participants