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

feat: add podcast list and spotify embedded #369

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

Conversation

kanhaiya88628
Copy link
Contributor

@kanhaiya88628 kanhaiya88628 commented Dec 21, 2022

Your checklist for this pull request

🚨Please review the guidelines for contributing to this repository.

  • Make sure you are requesting to pull a topic/feature/bugfix branch (right side). Don't request your default branch!
  • Make sure you are making a pull request against the default branch (left side). Also you should start your branch off default branch.
  • Check the commit's or even all commits' message styles matches our requested structure.
  • Check your code additions will fail neither code linting checks nor unit test.
  • I have added necessary documentation (if appropriate)

Linked Issues

Fixes #354

Description

Please describe your pull request.

❤️Thank you!

Post merge checklist

  • Follow steps from the guidelines for contributing to this repository.
  • If you are a new contributor, ping in the thread and one of the maintainers will add you to all-contributors list.

@120EE0692 120EE0692 added the enhancement New feature or request label Dec 22, 2022
Copy link
Member

@120EE0692 120EE0692 left a comment

Choose a reason for hiding this comment

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

Design nit matching, the selected podcast must be highlighted.

image

@120EE0692
Copy link
Member

@kanhaiya88628 more than requested time already assigned.

Copy link
Member

@120EE0692 120EE0692 left a comment

Choose a reason for hiding this comment

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

@kanhaiya88628

image

  1. remove the podcast navigate button from the expression page.
  2. on clicking seeAll it should redirect to first podcast page, which is presently redirecting to the sportify.
  3. add a little space between the play button and the podcast title.
  4. In the podcast page add next and previous text with the navigate button.
  5. remove the podcast placeholder file.
  6. undo the changes made in getSpotifyAccessToken.js file after finishing all your work.
  7. change title of the pull request.

@@ -1,7 +1,7 @@
const getSpotifyAccessToken = async () => {
try {
const { access_token } = await fetch(
process.env.NODE_ENV === 'production'
process.env.NODE_ENV !== 'production'
Copy link
Member

Choose a reason for hiding this comment

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

@kanhaiya88628 why this change committed

client/src/components/podcast/List.js Outdated Show resolved Hide resolved
@kanhaiya88628 kanhaiya88628 changed the title fix: podcast style feat: add podcast list and Spotify embedded Jan 25, 2023
@kanhaiya88628 kanhaiya88628 changed the title feat: add podcast list and Spotify embedded feat: add podcast list and spotify embedded Jan 28, 2023
@@ -30,7 +30,7 @@ function Expressions({
miscellaneous,
}) {
const classes = useStyles();

const hidePagination = true;
Copy link
Member

Choose a reason for hiding this comment

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

@kanhaiya88628 remove this.

@@ -70,7 +70,9 @@ function Expressions({
{
name: 'Podcasts',
container: true,
section: <PodcastList spotify={spotify} />,
section: (
<PodcastList spotify={spotify} hidePagination={hidePagination} />
Copy link
Member

Choose a reason for hiding this comment

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

@kanhaiya88628 no need create a variable directly use the required value i.e hidePagination={false}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kanhaiya88628 no need create a variable directly use the required value i.e hidePagination={false}

It is not working while declaring in this way

Copy link
Member

@120EE0692 120EE0692 left a comment

Choose a reason for hiding this comment

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

@kanhaiya88628 fix this issue as well.
image

@120EE0692
Copy link
Member

@kanhaiya88628 sync the code with the latest changes.

Copy link
Member

@120EE0692 120EE0692 left a comment

Choose a reason for hiding this comment

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

@kanhaiya88628 in previous first add icon then text and in next first text and then logo.
image

In first and last page button should be disabled not hidden.
image

Copy link
Member

@120EE0692 120EE0692 left a comment

Choose a reason for hiding this comment

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

expression subcategory page in getStaticPaths function add a filter function in paths to remove podcast page.

Comment on lines 17 to 18
const len = isNextNull.length;

Copy link
Member

Choose a reason for hiding this comment

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

@kanhaiya88628 add a null check here using chaining operator

@120EE0692
Copy link
Member

@kanhaiya88628 next and previous button is not deactivated at the beginning and at the end of the list. When a user clicks the next button when the list ends it shows the error page.

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

styles: fix podcast styling
2 participants