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

#383 Ayah Play Audio + Redux Refactoring (WIP) #390

Merged
merged 7 commits into from
Jul 10, 2016

Conversation

thabti
Copy link
Contributor

@thabti thabti commented Jul 10, 2016

Hi,

This will be a slightly fat PR. I am not a fan of the decorators, I will make a suggested change and let me know if you agree. I find wrapping the component in a normal function is much easier for new contributors who probably don't use bleed-edge of ES. Also they have not made it into the official spec. Thoughts?

  • Clicking the Play link should play the current ayah.
  • Refactoring out decorators
  • Hide play in search results

@ahmedre
Copy link
Contributor

ahmedre commented Jul 10, 2016

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

@ahmedre
Copy link
Contributor

ahmedre commented Jul 10, 2016

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

@ahmedre
Copy link
Contributor

ahmedre commented Jul 10, 2016

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

@ahmedre
Copy link
Contributor

ahmedre commented Jul 10, 2016

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

@ahmedre
Copy link
Contributor

ahmedre commented Jul 10, 2016

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

@ahmedre
Copy link
Contributor

ahmedre commented Jul 10, 2016

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

@ahmedre
Copy link
Contributor

ahmedre commented Jul 10, 2016

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

@ahmedre
Copy link
Contributor

ahmedre commented Jul 10, 2016

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

stop();
setAyah(ayah);
start();

Copy link
Contributor

Choose a reason for hiding this comment

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

✂️

@mmahalwy
Copy link
Contributor

@sabeurthabti this is awesome mashallah! Thanks for this

re decorators: i do really like how you separated the mapStateToProps methods, should definitely do that myself too. As for decorators, I do see your point about them being bleeding edge. One thing that separating them will benefit us is testability, more specifically see: http://redux.js.org/docs/recipes/WritingTests.html#connected-components

I do see your point but I really do like decorators, so I am torn. I think we should make a call and go with it. What do you think?

@ahmedre
Copy link
Contributor

ahmedre commented Jul 10, 2016

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

@thabti
Copy link
Contributor Author

thabti commented Jul 10, 2016

@mmahalwy let's discuss it on slack. I am happy with either. For me the main thing is readability.

@ahmedre
Copy link
Contributor

ahmedre commented Jul 10, 2016

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

@mmahalwy mmahalwy merged commit c2c1e64 into quran:develop Jul 10, 2016
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.

3 participants