Skip to content
This repository has been archived by the owner on Jun 28, 2021. It is now read-only.

Issue #471: better handling of sura/ayah urls #475

Merged
merged 4 commits into from
Sep 10, 2016

Conversation

atalebagha
Copy link
Contributor

@atalebagha atalebagha commented Sep 10, 2016

Related Issue

#471

Changelog

  • Routes with colons /39:4 are rerouted to /39/4
  • Inverted ranges are fixed /39/5-4 fixed to /39/4-5
  • Url with a range of 1 just prints 1 ayah /39:5-5 fixes to /39/5

@ahmedre
Copy link
Contributor

ahmedre commented Sep 10, 2016

Deployed to: http://staging.quran.com:32852

@ahmedre
Copy link
Contributor

ahmedre commented Sep 10, 2016

Deployed to: http://staging.quran.com:32853

@ahmedre
Copy link
Contributor

ahmedre commented Sep 10, 2016

Deployed to: http://staging.quran.com:32854

@hassansin
Copy link
Contributor

Quick thought: for handling redirection, you could replace these lines (https://github.com/quran/quran.com-frontend/blob/master/src/routes.js#L27-L31) with built-in Redirect component like <Redirect from="/:surahId:(:range)" to="/:surahId(/:range)" /> . then in isValidSurah you only need to handle the inverted range

@atalebagha
Copy link
Contributor Author

Thanks, will do!

@ahmedre
Copy link
Contributor

ahmedre commented Sep 10, 2016

Deployed to: http://staging.quran.com:32855

@thabti
Copy link
Contributor

thabti commented Sep 10, 2016

LGTM

@thabti
Copy link
Contributor

thabti commented Sep 10, 2016

@atalebagha Thank you bro!

@@ -4,4 +4,16 @@ export default function isValidSurah(nextState, replaceState) {
if (isNaN(surahId) || surahId > 114 || surahId < 1) {
replaceState('/error/invalid-surah');
}

if (nextState.params.range) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would love for this to be unit tested, inshallah in the future.

@thabti thabti merged commit 8b54642 into quran:master Sep 10, 2016
@atalebagha atalebagha deleted the feature/ayahParamHandling branch September 10, 2016 22:54
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants