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

$500 - Audit forms and fix inconsistencies with focus, tab and shift + tab behavior 4/4 #7918

Closed
5 tasks
dylanexpensify opened this issue Feb 25, 2022 · 125 comments
Closed
5 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Engineering External Added to denote the issue can be worked on by a contributor Monthly KSv2 Reviewing Has a PR in review Task

Comments

@dylanexpensify
Copy link
Contributor

dylanexpensify commented Feb 25, 2022

We should audit all our forms and fix any inconsistencies with focus, tab, shift + tab and enter behavior. The expected behavior is as follows:

  1. Tab navigates to the next input.
  2. Shift + tab navigates to the previous input.
  3. Enter submits the form.
  4. Space toggles checkboxes/dropdowns.

Note: We should make sure that tabbing cycles through the form in an order that makes sense, usually top to bottom.

Here's a list of forms to be audited:

@dylanexpensify dylanexpensify added Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Task labels Feb 25, 2022
@MelvinBot
Copy link

Triggered auto assignment to @MitchExpensify (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

@MelvinBot
Copy link

Triggered auto assignment to @MonilBhavsar (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@dylanexpensify
Copy link
Contributor Author

Note: decision to split this main issue up into smaller issues came from this convo

@MitchExpensify
Copy link
Contributor

How does that change the normal contributor management process @dylanexpensify ? Thanks

@MelvinBot
Copy link

@MonilBhavsar, @MitchExpensify Whoops! This issue is 2 days overdue. Let's get this updated quick!

@MitchExpensify
Copy link
Contributor

Bumping the above @dylanexpensify

@MelvinBot MelvinBot removed the Overdue label Mar 1, 2022
@dylanexpensify
Copy link
Contributor Author

Ah, we basically decided to break the issues into four smaller issues since we weren't sure about milestones. So now just proceed as normal, posting onto Upwork, adding exported, etc!

@MonilBhavsar
Copy link
Contributor

Is this a part of "New Expensify forms"?

@MitchExpensify
Copy link
Contributor

Upwork Job

@botify botify removed the Daily KSv2 label Mar 1, 2022
@MelvinBot MelvinBot added the Weekly KSv2 label Mar 1, 2022
@MelvinBot
Copy link

Triggered auto assignment to Contributor-plus team member for initial proposal review - @parasharrajat (Exported)

@MelvinBot MelvinBot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Mar 1, 2022
@MelvinBot
Copy link

Triggered auto assignment to @pecanoro (Exported), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

@MonilBhavsar MonilBhavsar removed their assignment Mar 2, 2022
@mdneyazahmad
Copy link
Contributor

Proposal

I will wrap each modal page in FocusTrap component such that when modal opens. It does not focus elements outside of modal. And store the reference of last element clicked to open the modal so that when modal closes it focus on that element. And of course audit and fix inconsistency.

I did it only for the ProfilePage Here is the implementation of FocusTrap Component Wrapper.

import React, {useRef, useEffect} from 'react';
import {View} from 'react-native';
import PropTypes from 'prop-types';
import FocusTrapLib from 'focus-trap-react';

import { useIsFocused } from '@react-navigation/native';

const propTypes = {
    children: PropTypes.node.isRequired,
};


function FocusTrap(props) {
    const isFocused = useIsFocused();
    const lastFocusedElement = useRef(null);

    useEffect(() => {
        if(isFocused) {
            lastFocusedElement.current = document.activeElement;
            return;
        }

        if(lastFocusedElement.current) {
            lastFocusedElement.current.focus();
        }

    }, [isFocused]);

    return (
        <FocusTrapLib active={isFocused}>
            <View>
                {props.children}
            </View>
        </FocusTrapLib>
    );
}

FocusTrap.propTypes = propTypes;
FocusTrap.displayName = 'FocusTrap';

export default FocusTrap;

Result

Screen.Recording.2022-03-10.at.1.13.53.AM.mov

@mdneyazahmad
Copy link
Contributor

This is not the final code there will be some refactoring. This is the base minimum to propose a solution. And also every other components will be refactored the same way.

@melvin-bot melvin-bot bot added the Overdue label Nov 28, 2022
@pecanoro
Copy link
Contributor

pecanoro commented Nov 28, 2022

@mdneyazahmad what's the last update on this?

@melvin-bot melvin-bot bot removed the Overdue label Nov 28, 2022
@mdneyazahmad
Copy link
Contributor

@pecanoro waiting on #12370. This will be merged soon, then we can work this issue. Thanks!

@Beamanator Beamanator changed the title [HOLD #10965] $500 - Audit forms and fix inconsistencies with focus, tab and shift + tab behavior 4/4 [HOLD #12370] $500 - Audit forms and fix inconsistencies with focus, tab and shift + tab behavior 4/4 Dec 20, 2022
@JmillsExpensify JmillsExpensify changed the title [HOLD #12370] $500 - Audit forms and fix inconsistencies with focus, tab and shift + tab behavior 4/4 [HOLD WAQ] $500 - Audit forms and fix inconsistencies with focus, tab and shift + tab behavior 4/4 Dec 27, 2022
@JmillsExpensify
Copy link

Still working on the linked PR, though in any case, I don't think this is a focus for WAQ per this Slack thread. Adding a hold as a result.

@melvin-bot melvin-bot bot added the Overdue label Dec 29, 2022
@pecanoro
Copy link
Contributor

Still on HOLD

@melvin-bot melvin-bot bot added Reviewing Has a PR in review and removed Overdue labels Dec 29, 2022
@JmillsExpensify
Copy link

Taking this off hold.

@JmillsExpensify JmillsExpensify changed the title [HOLD WAQ] $500 - Audit forms and fix inconsistencies with focus, tab and shift + tab behavior 4/4 $500 - Audit forms and fix inconsistencies with focus, tab and shift + tab behavior 4/4 Jan 17, 2023
@JmillsExpensify
Copy link

Note: Now that this is off hold, I think we have a decision before us. Do we do anything at all? I actually vote no, we shouldn't. I still don't think keyboard navigation is a priority – including but not limited to tab, shift + tab, etc. – so better to close this issue and move on. Please let me know if I'm missing something!

@melvin-bot

This comment was marked as off-topic.

@luacmartins
Copy link
Contributor

I think that's reasonable to me. The pages in this issue have either already changed (Settings - Profile) or will be changed significantly as part of passwordless (Settings - Change Password, Login form), which would leave us with:

  • Request a Call modal - seems to work fine and I didn't see any issues with focus, tab and shift + tab behavior
  • Add Personal Bank account flow - most of the flow is handled by Plaid, we only have a dropdown in our App. I didn't see any issues here either.

With that in mind, I think we might be good to close this one out!

@MitchExpensify
Copy link
Contributor

Agree with closing for all the reasons stated above!

@JmillsExpensify
Copy link

Thanks for hearing me out!

@mdneyazahmad
Copy link
Contributor

I have been assigned to this issue a long time ago, and have been constantly working on it. As this is a part of a larger issue broken into 4. I am assigned 3 of these including this. There are some challenges to fix the issues, and there have been some discussions in the past that took a long time. There are two PR raised that fixes this issue #8874 and #12370. I think contributor involved in this issue should be paid. Thank you!

@parasharrajat
Copy link
Member

Payment for this issue is still pending. There was multiple PR involved for these issues titled Audit forms and fix inconsistencies with focus, tab, and shift + tab behavior. I agree with @mdneyazahmad that a lot of research and time was spent on finding these solutions.

@pecanoro
Copy link
Contributor

pecanoro commented Feb 3, 2023

@MitchExpensify Can you open an internal discussion about the above in Slack? 🙇‍♀️

@MitchExpensify
Copy link
Contributor

MitchExpensify commented Feb 6, 2023

Ok to summarize how this played out to make sure I'm following:

Audit form inconsistencies were broken into four issue:

  1. Audit forms and fix inconsistencies with focus, tab and shift + tab behavior 1/4
  2. Audit forms and fix inconsistencies with focus, tab and shift + tab behavior 2/4
  3. Audit forms and fix inconsistencies with focus, tab and shift + tab behavior 3/4
  4. Audit forms and fix inconsistencies with focus, tab and shift + tab behavior 4/4

@mdneyazahmad is assigned to 2, 3, & 4 and received/will be receiving compensation for 2 & 3. With regards to compensation for 4 (this issue): We ultimately decided to do nothing but work was completed (e.g. #8874 and #12370) that fixed inconsistencies flagged in the issue. As such, and for the work throughout being assigned to this issue, we need to agree on a fair figure.

Does that summarize where we're at? Just want to be sure we're on the same page before posting in Slack to land on the fair figure

@mdneyazahmad
Copy link
Contributor

@MitchExpensify first of all all of these issues are of similar nature, maybe there was not require to break into 4, but it was broken into 4 to keep it simple.

  1. As these are different issue, I think the price should not be adjusted with others otherwise no need to break.
  2. As you can see the amount paid for 2/4 is just 250, but @parasharrajat better knows the effort and time required to complete that issue. Still I did not ask for a raise, because I thought, I can adjust with others.
  3. The task required to do in this issue is to audit and fix any inconsistency. I have done the audit in the past. As these issues are similar. It has has to wait for the common PR. I would have started the 1st PR from any issue. But I thought 2/4 was little easy, therefore PR primarily addresses that issue.
  4. We can also keep in mind the time this issues take.

I think the original price for this issue is justified. Maybe @parasharrajat has some input.

Thank you

@parasharrajat
Copy link
Member

I definitely agree with @mdneyazahmad. Technically, no PR was done for #7917 but it is still priced at 2k. Main work was done for 2 and 4. 3 was put on hold during the PR review.

So I think 500 for C+ in this issue is less. But I am fine with it as it was the price when the job was assigned (based on the process).

@MitchExpensify
Copy link
Contributor

Thank you both! I appreciate that extra context a lot. I've proposed this compensation in Slack and will issue payment accordingly!

@MitchExpensify
Copy link
Contributor

MitchExpensify commented Feb 8, 2023

Paid @parasharrajat via bonus on the old Upwork Job contract.

I've invite you, @mdneyazahmad, to the new Upwork posting here for payment seeing as the old one closed. Lemme know when you've accepted! Thanks for your patience

@mdneyazahmad
Copy link
Contributor

@MitchExpensify accepted!

@MitchExpensify
Copy link
Contributor

Awesome! Paid and contract ended. Thanks again everyone!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Engineering External Added to denote the issue can be worked on by a contributor Monthly KSv2 Reviewing Has a PR in review Task
Projects
None yet
Development

No branches or pull requests