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

refactor(web): use queries for dealing with questions #1516

Merged
merged 11 commits into from
Aug 2, 2024

Conversation

dgdavid
Copy link
Contributor

@dgdavid dgdavid commented Jul 30, 2024

In the context of #1439, replace Questions client by its equivalent using TanStack Query.

Additionally, it includes two commits with changes out of the scope of the PBI we're working on. Namely:

  • Migrates the components/questions files to TypeScript, 560f953
  • Stop re-exporting everything in components/questions/index.ts, 7c7b049

Tested manually with a LuksActivationQuestion

Screenshot from 2024-08-02 13-45-36


Related to https://trello.com/c/8u1WOJz4 (internal link)

@dgdavid dgdavid marked this pull request as ready for review July 31, 2024 12:52
Comment on lines +23 to +25
import GenericQuestion from "~/components/questions/GenericQuestion";
import QuestionWithPassword from "~/components/questions/QuestionWithPassword";
import LuksActivationQuestion from "~/components/questions/LuksActivationQuestion";
Copy link
Contributor Author

@dgdavid dgdavid Jul 31, 2024

Choose a reason for hiding this comment

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

We've agreed on not exporting all components in index file ( see https://trello.com/c/c0GEUMQ8 (internal link) and #1049). And this is a little step towards such an agreement. However, looking at it, two questions comes to my mind:

  • Should we use ~/components/<namespace>/<Component> or ./<Component>. I see the first approach kind of safe and stable, since it does not matter from where you are importing the component.

  • If we use ``~/components//does make sense to keep exporting _public_ components in the index file. For example, for importingQuestions.tsx` we could

    import Questions from "~/components/questions/Questions";

    or

     import { Questions } from "~/components/questions";

Honestly, for namespaces exporting a single component there is not too much difference / advantages. Another story is when importing multiple components from a single namespace, like core. Similar to what happens when importing components from PatternFly: it would be really annoying if we have had to import each component from its path.

Just sharing thoughts. No strong opinion here (yet 😝)

Comment on lines +23 to +25
import GenericQuestion from "~/components/questions/GenericQuestion";
import QuestionWithPassword from "~/components/questions/QuestionWithPassword";
import LuksActivationQuestion from "~/components/questions/LuksActivationQuestion";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This three import also reminds me the article I shared on slack channel two weeks ago about naming: https://kyleshevlin.com/prefer-noun-adjective-naming

It isn't exactly the same case than the article, but maybe going for QuestionGeneric, QuestionWithPassword, and QuestionLuksActivation would make them more consistent from a naming POV. Again, no strong opinion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or maybe we could just drop the Question prefix/suffix since they are already in the questions namespace.

@dgdavid dgdavid requested a review from imobachgs July 31, 2024 13:41
@coveralls
Copy link

coveralls commented Aug 1, 2024

Coverage Status

coverage: 70.959%. remained the same
when pulling c8779b7 on use-query-questions
into 9fe32ae on master.

Copy link
Contributor

@imobachgs imobachgs left a comment

Choose a reason for hiding this comment

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

It looks almost good. Thanks!

web/src/components/questions/LuksActivationQuestion.tsx Outdated Show resolved Hide resolved
web/src/components/questions/LuksActivationQuestion.tsx Outdated Show resolved Hide resolved
web/src/components/questions/QuestionActions.tsx Outdated Show resolved Hide resolved
web/src/components/questions/QuestionActions.tsx Outdated Show resolved Hide resolved
web/src/components/questions/QuestionActions.tsx Outdated Show resolved Hide resolved
web/src/components/questions/Questions.test.tsx Outdated Show resolved Hide resolved
Co-authored-by: Imobach González Sosa <[email protected]>
By using PF/Stack#hasGutter for adding vertical space between internal components.
@dgdavid dgdavid requested a review from imobachgs August 2, 2024 13:25
@dgdavid dgdavid merged commit b6de3f1 into master Aug 2, 2024
2 checks passed
@dgdavid dgdavid deleted the use-query-questions branch August 2, 2024 13:57
@imobachgs imobachgs mentioned this pull request Sep 20, 2024
imobachgs added a commit that referenced this pull request Sep 20, 2024
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.

3 participants