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

Flag improvements/tweaks #18

Merged
merged 12 commits into from Feb 7, 2020
Merged

Flag improvements/tweaks #18

merged 12 commits into from Feb 7, 2020

Conversation

@imorland
Copy link
Contributor

imorland commented Oct 23, 2019

  • Adds the option to allow users to flag their own posts (off by default, maintaining current behaviour)
    image

  • Increase the size of the flagPostModal from small to medium
    image

  • 'off-topic', 'inappropriate' and 'spam' now have an optional extra details textarea for providing additional context, if required.
    image

  • 'other' - submit remains disabled until extra details are entered
    image
    image

  • Column type change on flags - reason_details changed from varchar to text to allow for longer details input

All changes tested locally, and on giffgaff staging environment.

Requires flarum/lang-english #148

Copy link
Member

franzliedke left a comment

Thanks for your initiative!

We discussed this internally, and want to request a bunch of changes.

Most importantly, please avoid changes to the dist JS files, and please don't commit the composer.lock file. More comments inline...

src/Api/Serializer/FlagSerializer.php Outdated Show resolved Hide resolved
src/Listener/AddFlagsApiAttributes.php Outdated Show resolved Hide resolved
js/src/forum/components/FlagPostModal.js Outdated Show resolved Hide resolved
js/src/forum/components/FlagPostModal.js Outdated Show resolved Hide resolved
@imorland

This comment has been minimized.

Copy link
Contributor Author

imorland commented Nov 22, 2019

Thanks for reviewing @franzliedke, please see some questions inline above :)

I'll get a ticket raised on our side to work on the changes ASAP...

@franzliedke

This comment has been minimized.

Copy link
Member

franzliedke commented Jan 10, 2020

@imorland Have you had a chance to schedule this? Would be cool if we could get this into the upcoming beta.12 release.

If it helps, you can also split up the various parts of this, so that we can get at least parts of it into the release. The "Allow users to flag their own posts" feature, for example, seems to be done already.

@imorland

This comment has been minimized.

Copy link
Contributor Author

imorland commented Jan 10, 2020

@franzliedke Sorry for the delay on this one. I think the latest commit covers everything discussed here...

@imorland

This comment has been minimized.

Copy link
Contributor Author

imorland commented Jan 10, 2020

I still need to add flarum-flags.forum.post.reason-needed to lang-english, I'll do that if you're happy with the rest of the changes :)

Copy link
Member

franzliedke left a comment

Thanks for the quick response! We're getting there... 👍

One thing here, and others inline:

  • Please don't commit changes to the JS dist files. Our bot will take care of that when merging to master.
js/src/forum/components/FlagPostModal.js Outdated Show resolved Hide resolved
src/Command/CreateFlagHandler.php Outdated Show resolved Hide resolved
@franzliedke

This comment has been minimized.

Copy link
Member

franzliedke commented Jan 10, 2020

Could you share a screenshot of what the error handling will look like when I try to submit without entering a message?

@imorland

This comment has been minimized.

Copy link
Contributor Author

imorland commented Jan 10, 2020

@franzliedke the js dist files were removed in my last commit..

Screenshot of the error:
image

@imorland

This comment has been minimized.

Copy link
Contributor Author

imorland commented Jan 10, 2020

PR on lang-english has now also been updated

Copy link
Member

franzliedke left a comment

the js dist files were removed in my last commit..

Please don't delete them, but rather restore their state before this PR. 😉

@@ -46,6 +46,12 @@ export default class FlagPostModal extends Modal {
<input type="radio" name="reason" checked={this.reason() === 'off_topic'} value="off_topic" onclick={m.withAttr('value', this.reason)}/>
<strong>{app.translator.trans('flarum-flags.forum.flag_post.reason_off_topic_label')}</strong>
{app.translator.trans('flarum-flags.forum.flag_post.reason_off_topic_text')}
{this.reason() === 'off_topic' ? (
<div>
<label>{app.translator.trans('flarum-flags.forum.flag_post.optional_details')}</label>

This comment has been minimized.

Copy link
@franzliedke

franzliedke Jan 10, 2020

Member

Now the label needs to be linked to the textarea. 😉

This comment has been minimized.

Copy link
@franzliedke

franzliedke Jan 10, 2020

Member

Also, I'd prefer if we could move this block outside of the surrounding <label class="checkbox">.

This comment has been minimized.

Copy link
@imorland

imorland Jan 10, 2020

Author Contributor

So sorry - extreme tiredness is kicking in, only 5 hours sleep in 2 days :(

I need to take a break from this for the moment, will pick it back up over the weekend...

This comment has been minimized.

Copy link
@imorland

imorland Jan 10, 2020

Author Contributor

@franzliedke just having a second think, rather than having a label for the textarea, to save some screen real estate, how about using the textarea's placeholder

Example:
image

@franzliedke

This comment has been minimized.

Copy link
Member

franzliedke commented Jan 10, 2020

Thanks for the screenshot! I guess we should strive for moving this kind of form-related error message inline into the form, but that's a more general problem for all of Flarum, so don't worry about it here.

@franzliedke

This comment has been minimized.

Copy link
Member

franzliedke commented Jan 10, 2020

Actually... the Modal base class supports error alerts. Can you try using that for the error message? You can check the LoginModal for an example...

@imorland

This comment has been minimized.

Copy link
Contributor Author

imorland commented Jan 10, 2020

Yep, the modal error looks and feels much better for this case. Example:
image

@franzliedke

This comment has been minimized.

Copy link
Member

franzliedke commented Jan 11, 2020

So sorry - extreme tiredness is kicking in, only 5 hours sleep in 2 days :(

I need to take a break from this for the moment, will pick it back up over the weekend...

Flarum - stealing your sleep since 2015.

In all seriousness, please don't feel obliged to give us your precious weekend time, or time you could be sleeping. There's still several weeks left until the next release. 😃

@tariqwalji

This comment has been minimized.

Copy link

tariqwalji commented Jan 30, 2020

Hi @franzliedke ,

Do you know what else is outstanding in order to get this merged in?

Copy link
Member

franzliedke left a comment

Hmm, I must have missed that all the changes were already made. Sorry about that. Just a little thing left over now...

js/src/forum/addFlagControl.js Outdated Show resolved Hide resolved
imorland added 2 commits Feb 1, 2020
@imorland

This comment has been minimized.

Copy link
Contributor Author

imorland commented Feb 1, 2020

@franzliedke moved canFlag server side (I don't know why I didn't think of that straight away, sorry)

Also removed post.user() === app.session.user from JS, as this check is now carried out BE.

@imorland imorland requested a review from franzliedke Feb 3, 2020
Copy link
Member

franzliedke left a comment

Thanks for making the changes! I'll merge.

@franzliedke franzliedke merged commit 5292e6c into flarum:master Feb 7, 2020
2 checks passed
2 checks passed
WIP Ready for review
Details
continuous-integration/styleci/pr The analysis has passed
Details
@imorland

This comment has been minimized.

Copy link
Contributor Author

imorland commented Feb 7, 2020

Thank you @franzliedke

@franzliedke

This comment has been minimized.

Copy link
Member

franzliedke commented Feb 7, 2020

Thank you!

I'll make some minor tweaks in master, and also change the translation keys - that felt faster than dumping another round of review on you. 😉

franzliedke added a commit to flarum/lang-english that referenced this pull request Feb 7, 2020
Refs flarum/flags#18.
Closes #148.
franzliedke added a commit that referenced this pull request Feb 7, 2020
Refs #18.
franzliedke added a commit that referenced this pull request Feb 7, 2020
This also gives the server-side the chance to add more different error
messages / additional behavior without having to change the frontend.

Refs #18.
franzliedke added a commit that referenced this pull request Feb 7, 2020
Refs #18.
franzliedke added a commit that referenced this pull request Feb 7, 2020
Refs #18.
@franzliedke

This comment has been minimized.

Copy link
Member

franzliedke commented Feb 7, 2020

@imorland I'm done. I tweaked the translation keys for consistency, removed the custom modal error handling (which basically reimplemented the default, but was less flexible) and fixed two little bugs. 😁

Nice work overall!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.