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

Scroll to edited post or inform the user #2108

Merged
merged 2 commits into from
May 31, 2020
Merged

Scroll to edited post or inform the user #2108

merged 2 commits into from
May 31, 2020

Conversation

the-turk
Copy link
Contributor

@the-turk the-turk commented Mar 31, 2020

Fixes #634

Changes proposed in this pull request:
Pretty much copy/pasted from ReplyComposer.

If we're currently viewing the discussion which post edit was made in, then we can scroll to the post. Otherwise, we'll create an alert message to inform the user that their edit has been made, containing a button which will transition to their edited post when clicked.

Reviewers should focus on:

  • Translations.
  • Did I miss something?

Confirmed

  • Frontend changes: tested on a local Flarum installation.
  • Backend changes: tests are green (run composer test).

Required changes:

@franzliedke
Copy link
Contributor

@the-turk Thanks for another nice PR! Keep 'em coming. 👍

As you can see, we have a lot of open PRs currently. Our top goal right now is to keep our focus on the stable release, and our planned path to get there.

As this issue (#634) was originally planned for a later release, it might take a while until we get around to further reviews (and merging it). Now that so much work has already been done, that does not mean we cannot merge a good PR if we find the time, but I wanted to explain what we're trying to keep in mind when choosing what to review next, in order to avoid confusion or disappointment. We are trying hard not to get sidetracked in our focus, and our resources are limited.

Thanks for your understanding - and your contribution! 🤗

@the-turk
Copy link
Contributor Author

Thanks @franzliedke, I appreciate that! I'm aware of your priorities and hard work. I just wanted to lighten the load on y'all in my free time and these are just my humble contributions. I'll leave it here for future reference but I should say that I'm not happy with using the same code multiple times either, feel free to close it if you've got better ideas to add the feature. Thanks again. 🤜

@franzliedke
Copy link
Contributor

Because we now auto-format our JS code with Prettier, this branch now has conflicts with master. Sorry about that. 😉

Please take the steps outlined in the forum to resolve the conflicts.

Copy link
Sponsor Member

@askvortsov1 askvortsov1 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, just needs a rebase. Thanks!

@the-turk
Copy link
Contributor Author

the-turk commented May 23, 2020

Did I do this right? 🙈

what the hell am supossed to do with the lock file

@the-turk the-turk requested a review from askvortsov1 May 23, 2020 09:33
@dsevillamartin
Copy link
Member

@the-turk The lock file comes from npm install.

This is basically the same logic as the reply alert, right? Not sure if any of it can be moved into a common place so there's less duplication, if that's true.

@the-turk
Copy link
Contributor Author

hey @datitisev, that's true

I should say that I'm not happy with using the same code multiple times either, feel free to close it if you've got better ideas to add the feature. Thanks again. 🤜

@dsevillamartin
Copy link
Member

Not sure if we want to simply stuff in this PR or make it separate. 🤷 Either works for me.

@askvortsov1
Copy link
Sponsor Member

For this case in particular, I'm fine having the code structure repeated. It's only really used twice, if we have to add it again, then we can try to abstract it out. Additionally, there's enough differences between the two that abstracting out the logic would likely create a messier codebase, not a cleaner one

Copy link
Sponsor Member

@askvortsov1 askvortsov1 left a comment

Choose a reason for hiding this comment

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

I have checked this out and confirmed that it works locally. Thank you for the PR!

Copy link
Member

@clarkwinkelmann clarkwinkelmann left a comment

Choose a reason for hiding this comment

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

Just looking at the code, looking fine for me

@askvortsov1 askvortsov1 merged commit 0aed376 into flarum:master May 31, 2020
littlegolden added a commit to littlegolden/lang-english that referenced this pull request Oct 21, 2020
@dsevillamartin
Copy link
Member

We never merged the locale strings for this PR...

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.

Editing a post from another page
5 participants