Conversation
…e.(ts|tsx|jsx|js) as actual pages This will allow us to colocate the graphql and all the business components required in the pages folder
|
@nazarfil @yolanfery Could I have your impressions on this change ? |
yolanfery
left a comment
yolanfery
left a comment
There was a problem hiding this comment.
I am under the impression that :
- Overall this is an improvement (navigation in the project would be easier, and the definition of graphql would be consistent opening up for opportunities to simplify and optimize the queries by using existing queries as examples)
- Seems like a tedious manual process and I am not sure if it is worth the effort/risk (of regressions)
| @@ -25,7 +26,14 @@ interface LoginForm { | |||
|
|
|||
| const LoginPage: NextPageWithLayout = () => { | |||
| const router = useRouter(); | |||
| const [doLogin] = useLoginMutation(); | |||
There was a problem hiding this comment.
Is this particular change an improvement ? I am not sure about the direction of using inline qgl
There was a problem hiding this comment.
That's exactly why I wanted to have the opinion of the team. I started to move the graphQL operation inside a login.graphql and importing it in this file but I wanted to propose you both.
| const QUERY = gql` | ||
| query notebooksPage { | ||
| notebooksUrl | ||
| } | ||
| `; | ||
|
|
||
| const NotebooksPage = () => { | ||
| const { data } = useNotebooksPageQuery(); | ||
| const { data } = useQuery<NotebooksPageQuery>(QUERY); |
There was a problem hiding this comment.
Same for this one : Is this particular change an improvement ? I am not sure about the direction of using inline qgl
| import WebappIframe from "./WebappIframe"; | ||
|
|
||
| describe("WebappIframe", () => { | ||
| it("⚠️ should not allow same origin iframe attribute for url of same origin", () => { |
There was a problem hiding this comment.
(Those tests should stay obviously)
I am also under the impression that you also had a lot of fun for (seemingly) your last PR 😄 |
Renaming files and moving things is bringing so much joy :D
I feel that it's going to lower the cost of entry in the project. |
qgerome
force-pushed
the
refactor-frontend-graphql
branch
from
6671ed8 to
e1008e4
Compare
There was a problem hiding this comment.
@yolanfery is it better with a graphic file for the operations ?
3 participants
A short, meaningful PR description. Explain the goal of the PR and the ideas behind it.
Changes
Please list / describe the changes in the codebase for the reviewer(s).
How/what to test
A few words about what needs to be tested, along with specific testing instructions if needed.
Screenshots / screencast
If relevant, please add some screenshots or a short video.