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

Focus compose box when LHN report clicked #2032

Merged
merged 1 commit into from
Mar 24, 2021
+5 −2
Merged

Focus compose box when LHN report clicked #2032

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions src/pages/home/report/ReportActionCompose.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,8 +78,11 @@ class ReportActionCompose extends React.Component {
this.comment = this.props.comment;
}

// When any modal goes from visible to hidden, bring focus to the compose field
if (prevProps.modal.isVisible && !this.props.modal.isVisible) {
// When any modal goes from visible to hidden or when the report ID changes, bring focus to the compose field
if (
Copy link
Contributor

@sketchydroide sketchydroide Mar 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think we separate lines for ifs something like this is closer to our guidelines I believe

if ((prevProps.modal.isVisible && !this.props.modal.isVisible)
    || (prevProps.reportID !== this.props.reportID)) {

Would be otherwise happy :)

Copy link
Contributor

Choose a reason for hiding this comment

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

never mind that, clearly I have not worked on the this repo for a while 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was wondering what y'all like to do in these situations! haha

I got this style from: https://github.com/airbnb/javascript#control-statements

Here, they reference this example:

// good
if (
  (foo === 123 || bar === 'abc')
  && doesItLookGoodWhenItBecomesThatLong()
  && isThisReallyHappening()
) {
  thing1();
}

Anyway, let me know what you think! I'm happy to modify :D

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, you are tottally right, sorry for that, I haven't open this repo in a while, this does seem like the way we do it in this project, keeping consitency is important, lets continue to do that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok great, thanks for asking anyway!

(prevProps.modal.isVisible && !this.props.modal.isVisible)
|| (prevProps.reportID !== this.props.reportID)
) {
this.setIsFocused(true);
}
}
Expand Down