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.

Sign up for GitHub

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

Jump to bottom

feat(frontend): add a back to results link #542

Merged
merged 1 commit into from
Feb 26, 2019
Merged

feat(frontend): add a back to results link #542

merged 1 commit into from
Feb 26, 2019

Conversation

UnbearableBear
Copy link
Contributor

@UnbearableBear UnbearableBear commented Feb 21, 2019 •
edited by douglasduteil
Loading

C'est minimaliste mais fonctionnel !

Closes #527

@codecov
Copy link

codecov bot commented Feb 21, 2019 •
edited
Loading

Codecov Report

Merging #542 into master will increase coverage by 1.48%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #542      +/-   ##
==========================================
+ Coverage   72.79%   74.27%   +1.48%     
==========================================
  Files          93      119      +26     
  Lines        1077     1213     +136     
  Branches      167      184      +17     
==========================================
+ Hits          784      901     +117     
- Misses        263      278      +15     
- Partials       30       34       +4
Flag Coverage Δ
#true 72.77% <66.66%> (-0.02%) ⬇️
Impacted Files Coverage Δ
...ages/code-du-travail-frontend/src/common/Answer.js 58.33% <66.66%> (+2.77%) ⬆️
packages/code-du-travail-ui/src/Card.js 100% <0%> (ø)
...ode-du-travail-data/dataset/tag_extractor/index.js 85% <0%> (ø)
packages/code-du-travail-ui/src/Categories.js 100% <0%> (ø)
packages/code-du-travail-ui/src/LargeLink.js 100% <0%> (ø)
packages/code-du-travail-ui/src/Section.js 100% <0%> (ø)
packages/code-du-travail-ui/src/icons/Search.js 100% <0%> (ø)
packages/code-du-travail-ui/src/Container.js 100% <0%> (ø)
packages/code-du-travail-ui/src/NoAnswer.js 100% <0%> (ø)
packages/code-du-travail-ui/src/IconButton.js 100% <0%> (ø)
... and 17 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cbea80c...8f7fa19. Read the comment docs.

@revolunet
Copy link
Member

styled(Link) aurait suffi ?

@UnbearableBear
Copy link
Contributor Author

UnbearableBear commented Feb 21, 2019 •
edited
Loading

Ah je ne savais pas que je pouvais faire ça, c'est cool ça enlève une div !

Par contre je viens de tester et j'ai l'impression que ça ne fonctionne pas avec ce component là :(
Ils disent dans la doc de Styled Components:

as long as they attach the passed className prop to a DOM element.

source

Et alors que là à priori ça devrait passer -> https://github.com/SocialGouv/next-routes/blob/master/src/index.js#L105
Là ils disent clairement que ça ne passera pas -> vercel/next.js#1642

Mais ils proposent ça en solution et ça fonctionne ! vercel/next.js#1942 (comment)

@UnbearableBear
Copy link
Contributor Author

@revolunet changed, si ça te va, ça me va !

@UnbearableBear UnbearableBear force-pushed the manu/return-to-search-results branch from b49daaa to cf65b4b Compare February 21, 2019 10:58
douglasduteil
douglasduteil suggested changes Feb 21, 2019
View reviewed changes
packages/code-du-travail-frontend/src/common/Answer.js Outdated
@@ -14,6 +17,13 @@ const BigError = ({ children }) => (
</Container>
);

// manuC: Doing this feels stange, we should talk about it
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer https://www.conventionalcommits.org similar format <type>(<name>): <description> and use your Github username

TODO(UnbearableBear): Need to do this thing on my work in progress
NOTE(UnbearableBear): This code is here because ...
FIXME(UnbearableBear): Below code is bad and this is why
FUTURE(UnbearableBear): This code is temporary and will change in the future
BUG(UnbearableBear): There is a known bug here lol
HACK(UnbearableBear): This code is unnatural but do the trick because...

@UnbearableBear UnbearableBear force-pushed the manu/return-to-search-results branch from cf65b4b to 1701cd8 Compare February 21, 2019 13:35
@UnbearableBear
Copy link
Contributor Author

Changed !

@UnbearableBear UnbearableBear force-pushed the manu/return-to-search-results branch from 1701cd8 to 4d7c510 Compare February 25, 2019 14:39
lionelB
lionelB suggested changes Feb 25, 2019
View reviewed changes
packages/code-du-travail-frontend/src/common/Answer.js Outdated
href="/"
onClick={e => {
e.preventDefault();
window.history.back();
Copy link
Contributor

Choose a reason for hiding this comment

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

On decrait pouvoir reconstruire la page précedente avec la queryString
ca permettrait d'avoir un lien qui marche sans JS et qui fonctionnerait aussi en SSR.
Il faut par contre traiter le cas ou l'on a pas de queryString et dans ce cas ce serait un lien vers la home

@UnbearableBear UnbearableBear force-pushed the manu/return-to-search-results branch from 4d7c510 to fde6877 Compare February 25, 2019 17:38
douglasduteil
douglasduteil previously requested changes Feb 25, 2019
View reviewed changes
packages/code-du-travail-frontend/src/common/Answer.js Outdated
class Answer extends React.Component {
state = {
modalVisible: false
modalVisible: false,
isMounted: false
Copy link
Contributor

Choose a reason for hiding this comment

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

isMounted What is it for ?

Copy link
Contributor Author

@UnbearableBear UnbearableBear Feb 25, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Bordel j'ai déconné. It is useless !

lionelB
lionelB suggested changes Feb 25, 2019
View reviewed changes
packages/code-du-travail-frontend/src/common/Answer.js Outdated

return (
<Container>
<Link route="index" params={{ q: search }}>
Copy link
Contributor

Choose a reason for hiding this comment

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

il n'y a pas que le param q, est ce que tu peux passer directement query à la fonction pour avoir l'ensemble de param de query ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes bonne idée !!

packages/code-du-travail-frontend/src/common/Answer.js Outdated
class Answer extends React.Component {
state = {
modalVisible: false
modalVisible: false,
isMounted: false
Copy link
Contributor

Choose a reason for hiding this comment

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

cool de rajouter l'initailState,

  • il manque searchResults
  • isMounted à l'air de pas être utiliser,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes je fais ça de suite

@UnbearableBear UnbearableBear force-pushed the manu/return-to-search-results branch from fde6877 to 8f7fa19 Compare February 25, 2019 18:30
@UnbearableBear UnbearableBear merged commit a245875 into master Feb 26, 2019
@UnbearableBear UnbearableBear deleted the manu/return-to-search-results branch March 17, 2019 17:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lionelB lionelB lionelB approved these changes

@douglasduteil douglasduteil douglasduteil left review comments

@revolunet revolunet Awaiting requested review from revolunet

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@UnbearableBear @revolunet @lionelB @douglasduteil