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

Feature/solved issues #153

Merged
merged 85 commits into from
Apr 21, 2020
Merged

Feature/solved issues #153

merged 85 commits into from
Apr 21, 2020

Conversation

janikga
Copy link
Collaborator

@janikga janikga commented Mar 25, 2020

What changes does this PR introduce

  • This PR introduces the solved issues feature. Entries with responses can now be marked as solved by the owner, rather than deleting them directly. For this, tabs were introduced to the dashboard component, where a user can switch between open and closed entries.
  • Additionally, new Popup hints were introduced. For example, if a user tries to delete an open entry, he receives a hint that he can mark it as solved instead. Other popups, such as a general deletion reassurement popup and a deletion was successful popup, were introduced as well.
  • fixes button bar on mobile
  • skips unit tests for now, as they are flaky. Follow-up ticket: Refactor firebase interactions into single utility class #216

Checklist

  • yarn build passes
  • yarn lint does not show any errors

Others details

  • This PR introduces a new dependency. Reason:
    • react-tabs is needed for the tabs list in the dashboard component
    • reactjs-popup is needed for the popups
    • PR depends on the this PR for the updated firebase functions

Screenshots

Screenshot 2020-03-27 at 10 31 10

Screenshot 2020-03-27 at 10 35 31

Screenshot 2020-03-27 at 10 34 58

Screenshot 2020-03-27 at 10 35 12

Screenshot 2020-03-27 at 10 35 47

@janikga janikga self-assigned this Mar 25, 2020
src/components/Entry.jsx Outdated Show resolved Hide resolved
const deleteButtonClasses = `bg-red-200 text-primary ${commonButtonClasses}`;
const invertedDeleteButtonClasses = `text-primary font-medium min-w-90 ${commonButtonClasses.replace('font-bold', '')}`;

const InitializeDeletionButton = getButtonForPopup(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tried to make this as generic as possible, to avoid redundant code repetitions, since all of the buttons in the popup are similar. Please let me know if you have a better suggestion for how to do this!

Copy link
Member

Choose a reason for hiding this comment

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

In 'src/styles/index.css' there are some classes that use apply in order to combine styles from different classes into one. Maybe we could use something like this as well here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, however, it is more than just styling since we also specify a different onClick handling and icon for each button..

src/components/Entry.jsx Outdated Show resolved Hide resolved
src/components/Popup.jsx Outdated Show resolved Hide resolved
@janikga janikga changed the title WIP: Feature/solved issues Feature/solved issues Mar 27, 2020
@mauriceackel mauriceackel changed the base branch from develop to master March 27, 2020 17:21
@florianschmidt1994 florianschmidt1994 self-requested a review April 8, 2020 13:12
Copy link
Member

@florianschmidt1994 florianschmidt1994 left a comment

Choose a reason for hiding this comment

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

Looks good to me. I've added some minor design adjustments.

@florianschmidt1994
Copy link
Member

@kenodressel could you have a look as well?

package.json Outdated Show resolved Hide resolved
public/locales/de/translation.json Outdated Show resolved Hide resolved
"createNewRequest": "Neue Anfrage Erstellen",
"solveReassure": {
"heading": "War dein Hilfegesuch erfolgreich?",
"firstSentence": "Lass die Community wissen dass deine Suche erfolgreich war.",
Copy link
Member

Choose a reason for hiding this comment

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

Please check with the team on which capitalization for "Dein" and similar.

src/components/entry/Entry.jsx Show resolved Hide resolved
const doc = await fb.store.collection(collectionName).doc(props.id).get();
await fb.store.collection('/deleted').doc(props.id).set({
collectionName, ...doc.data(),
});
setDeleted(true);
setAttemptingToDelete(false);
setPopupVisible(true);
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be set to false here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

needs to be set to true to trigger the deletion confirmation popup, will add a comment!

Comment on lines 78 to 79
setDeleted(true);
setAttemptingToDelete(false);
Copy link
Member

Choose a reason for hiding this comment

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

Why do we set deleted here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we use the deleted state to hide/show the entry. Agree that it should be refactored to visible instead

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Edit: will introduce a second state solved instead

Comment on lines +148 to +165
const popupOnEntryAction = (
<PopupOnEntryAction
responses={responses}
attemptingToDelete={attemptingToDelete}
attemptingToSolve={attemptingToSolve}
deleted={deleted}
popupVisible={popupVisible}
setPopupVisible={setPopupVisible}
setAttemptingToDelete={setAttemptingToDelete}
handleSolved={handleSolved}
showAsSolved={showAsSolved}
handleNewAskForHelp={handleNewAskForHelp}
cancelDelete={cancelDelete}
handleDelete={handleDelete}
backToOverview={backToOverview}
/>
);

Copy link
Member

Choose a reason for hiding this comment

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

For me, this looks like a hack.. Shouldn't a generic dialog have some text to pass as well as some button types to pass and register a callback that reds the button type and executes the desired action?

@tgraupne
Copy link
Member

Hey team, what is the current state here?

@janikga
Copy link
Collaborator Author

janikga commented Apr 14, 2020

@Maurice22 requested changes and suggested to implement them himself, we are currently deciding whether this can be done as a follow up to this PR or if they are release critical and should be done beforehand :)

@mauriceackel
Copy link
Member

I probably won't be able to get this done soon, so I created #228 as my follow-up

@mauriceackel mauriceackel merged commit dd93655 into master Apr 21, 2020
@mauriceackel mauriceackel deleted the feature/solved-issues branch April 21, 2020 19:54
@mauriceackel
Copy link
Member

Work on #216 and #228 can be started now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs consideration Larger feature, needs sophisticated testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Set requests to "fullfilled" instead of "deleted"
4 participants