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

Connect home and discover screens to the backend #48

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

daniyelp
Copy link
Contributor

@daniyelp daniyelp commented Jan 18, 2022

What does it fix?

Resolves #38.

As a quick sum-up, I imported the GraphQL schema, created the repository to fetch the lessons and the lesson themes and updated the screens to the new model classes (*interfaces) created by Apollo.
It might not be ready for merging yet, because there were some things that got me confused / I wasn't quite sure of:

  • I created Fragments for the GraphQL models, for which Apollo generated interfaces and not data classes, that I later used as return types in the new Repository that I've created. Is it ok? Should I have used instead the data classes that were generated for each query? Sticking with my choice to use interfaces makes it also a little inconvenient to create fake data, which is why I deleted the fake repositories (but probably should bring back for testing).
  • Could there really be a case in which the id of a Lesson or LessonTheme might be null?
  • Instead of using just a Lesson in the LessonCard, I had to use a LessonStatus, because a Lesson doesn't contain the percentageCompleted field. This is inconvenient because in order to get all the data needed to display the lessons you would have to write two queries: one to fetch the ids of the lessons for the respective LessonTheme; a second query with the list of ids to get the list of LessonStatuses. Am I missing something?

@daniyelp daniyelp force-pushed the fetch-lessons-from-api branch from d309a7c to 264bcdd Compare January 18, 2022 13:35
@daniyelp daniyelp marked this pull request as ready for review January 24, 2022 14:02
@alexandru-calinoiu
Copy link
Contributor

Good point with the nullable id's, opened a issue with the backend code4romania/teacher-workout-backend#74

@alexandru-calinoiu
Copy link
Contributor

There is a small discussion that needs to happen here,

The fact that data and model are hosted in the commons/ui module does not look right, even name wise :)

The usual approach I see in the wild is for this model and data projects / modules to be shared in the entire app and host all repos / mappers and models of the app.
There are a couple of issues that this approach has that I will try to very briefly summarize:

  • this classes tend to get rather large, especially when it comes to using them with graphql, because one will select all fields in the query no matter where is used
  • the current project uses this domain driven approach, and it breaks the project into features, having the models / repos common kind of invalidates a lot of the benefits of this approach.

@alexandru-calinoiu
Copy link
Contributor

I've made a POC with a proposed approach to data access #50

Please take a look and let me know what you think.

@daniyelp
Copy link
Contributor Author

Good points at first. At the moment, I'm really caught into something and I'll try to take a deeper look into the PR Monday or Tuesday.

@daniyelp
Copy link
Contributor Author

daniyelp commented Feb 2, 2022

@alexandru-calinoiu As I said in the other PR, what I did doesn't seem like the the way to go. It's too much of a REST approach 😵. So I think that we should close this PR.
But regarding the last question I've asked, how would we fetch all the data needed for the home screen in one go?

@alexandru-calinoiu
Copy link
Contributor

@daniyelp one can write a query similar to this one to fetch all the data

query {
  lessonStatuses(lessonIds: ["1"]) {
    currentLessonStep {
      id
      
      ... on SlideStep {
        __typename
        title
        description
      }
      ... on ExerciseStep {
        __typename
      }
    }
  }
  themes {
    edges {
      node {
        id
        title
        thumbnail {
          url
        }
      }
    }
  }
}

But I think we need to open a ticket with the backend so that the lessonStatuses query will bring all lesson statuses for the current user.

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

Successfully merging this pull request may close these issues.

Connect lesson themes screen to API
2 participants