-
Notifications
You must be signed in to change notification settings - Fork 16
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
Conversation
src/components/Entry.jsx
Outdated
const deleteButtonClasses = `bg-red-200 text-primary ${commonButtonClasses}`; | ||
const invertedDeleteButtonClasses = `text-primary font-medium min-w-90 ${commonButtonClasses.replace('font-bold', '')}`; | ||
|
||
const InitializeDeletionButton = getButtonForPopup( |
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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..
There was a problem hiding this 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.
@kenodressel could you have a look as well? |
public/locales/de/translation.json
Outdated
"createNewRequest": "Neue Anfrage Erstellen", | ||
"solveReassure": { | ||
"heading": "War dein Hilfegesuch erfolgreich?", | ||
"firstSentence": "Lass die Community wissen dass deine Suche erfolgreich war.", |
There was a problem hiding this comment.
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
Outdated
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); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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!
src/components/entry/Entry.jsx
Outdated
setDeleted(true); | ||
setAttemptingToDelete(false); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
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} | ||
/> | ||
); | ||
|
There was a problem hiding this comment.
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?
Hey team, what is the current state here? |
@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 :) |
I probably won't be able to get this done soon, so I created #228 as my follow-up |
What changes does this PR introduce
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 thedashboard
component, where a user can switch between open and closed entries.Checklist
yarn build
passesyarn lint
does not show any errorsOthers details
dashboard
componentScreenshots