Simple refactorizations (v2). #838
Conversation
src/config.js
Outdated
| 'muslim', | ||
| 'islam', | ||
| 'Allah' | ||
| ]; |
There was a problem hiding this comment.
Allows us to more sensibly add keywords in the future.
| @@ -28,24 +46,43 @@ const config = { | |||
| sentryClient: process.env.SENTRY_KEY_CLIENT, | |||
| sentryServer: process.env.SENTRY_KEY_SERVER, | |||
| facebookAppId: process.env.FACEBOOK_APP_ID, | |||
| // Supported locales | |||
| locales, | |||
| defaultLocale: 'en', | |||
| app: { | |||
| head: { | |||
| titleTemplate: `%s - ${title}`, | |||
| meta: [ | |||
There was a problem hiding this comment.
All of the changes below are simply to make the file look more consistent, and future metadata additions would follow that one style.
| @@ -191,7 +308,7 @@ const config = { | |||
| }, | |||
| ...Object.keys(locales).map(key => ({ | |||
| rel: 'alternate', | |||
| hreflang: key, | |||
| hrefLang: key, | |||
There was a problem hiding this comment.
Apparently hreflang is a typo..?
src/containers/About/index.js
Outdated
| @@ -4,33 +4,33 @@ import Helmet from 'react-helmet'; | |||
|
|
|||
| export default () => ( | |||
| <div> | |||
There was a problem hiding this comment.
Figured we'd try and reference it as Qur'an more and more, but it may have been overkill in some places. In particular I think I'll revert the diff right below this comment after the tests pass -- please remind me before review approval & merge. Reverted that specific change.
| @@ -0,0 +1,118 @@ | |||
| /* eslint-disable no-unused-vars */ | |||
There was a problem hiding this comment.
We add this file to drop major constants like the one below, which may be reusable.
src/config.js
Outdated
| { rel: 'manifest', href: 'manifest.json' }, | ||
| { | ||
| rel: 'manifest', | ||
| href: 'manifest.json' |
There was a problem hiding this comment.
this should be /manifest.json
src/config.js
Outdated
| }, | ||
| { | ||
| rel: 'apple-touch-icon', | ||
| href: 'apple-touch-icon.png' |
There was a problem hiding this comment.
Refactoring brings up unforeseen issues 👍
src/containers/AyatulKursi/index.js
Outdated
| const description = | ||
| 'Ayatul Kursi is verse 255 of the second chapter, surah Baqarah of the Holy Quran, Surah al-Baqarah (The Chapter of the Cow). It is also known as the Throne Verse.'; | ||
| const description = 'Ayatul Kursi is verse 255 of the second chapter in the ' + | ||
| 'Holy Quran, Surah al-Baqarah (The Chapter of the Cow). ' + |
There was a problem hiding this comment.
second chapter, Surah al_Baqarah( The Chapter of Cow) of Holy Quran feel more readable :)
| replaceState('/error/invalid-surah'); | ||
| } | ||
| } | ||
|
|
||
| function filterValidVerse(replaceState, chapterId, verseRange) { | ||
| const chapterToVersesMap = { |
There was a problem hiding this comment.
+1 the best code is deleted code :)
|
@naveed-ahmad Thanks for the review. I've made the changes. |
|
rebuild |
|
rebuild |
1 similar comment
|
rebuild |
|
Deployed to: http://staging.quran.com:32904 |
Ensure consistent JSON-style maps. Add HTML corrections & reorganize a little. Correct href keys.
|
Deployed to: http://staging.quran.com:32906 |
|
Sorry about the build errors, @naveed-ahmad. It was a super tiny error that wasn't made clear from the logs :) All is good now and I've squashed the commits. |
|
Deployed to: http://staging.quran.com:32909 |
Assalamu Alaikum,
Here's a relatively tiny PR in a continued effort to try and refactor the codebase to be more consistent / clean / efficient, etc.
See my comments within the diffs for info on certain changes.
Reviewers