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

Refactor FXIOS-7301 - Reduce from 390 to 42 lines a property of AddressBarState.swift - help in closure_body_length #22711

Open
wants to merge 55 commits into
base: main
Choose a base branch
from
Open

Refactor FXIOS-7301 - Reduce from 390 to 42 lines a property of AddressBarState.swift - help in closure_body_length #22711

wants to merge 55 commits into from

Conversation

ionixjunior
Copy link
Contributor

@ionixjunior ionixjunior commented Oct 22, 2024 •
edited
Loading

📜 Tickets

Jira ticket
Github issue

💡 Description

This PR reduces from 390 to 42 lines of the reducer property from the AddressBarState.swift file by extracting the switch case statements to new functions.

The current threshold is 30, and I wasn't able to meet that in this PR. We might need to consider adjusting the threshold or allowing exceptions when we enable this rule. I'm open to any suggestions.

This PR is part of a series of PRs described here #16442 (comment).

📝 Checklist

You have to check all boxes before merging

  • Filled in the above information (tickets numbers and description of your work)
  • Updated the PR name to follow our PR naming guidelines
  • Wrote unit tests and/or ensured the tests suite is passing
  • When working on UI, I checked and implemented accessibility (minimum Dynamic Text and VoiceOver)
  • If needed, I updated documentation / comments for complex code and public methods
  • If needed, added a backport comment (example @Mergifyio backport release/v120)

@ionixjunior ionixjunior requested a review from a team as a code owner October 22, 2024 22:44
@mergify Mergify
Copy link
Contributor

mergify bot commented Oct 23, 2024

This pull request has conflicts when rebasing. Could you fix it @ionixjunior? 🙏

@FilippoZazzeroni FilippoZazzeroni requested review from FilippoZazzeroni and removed request for MattLichtenstein October 23, 2024 09:23
FilippoZazzeroni
FilippoZazzeroni reviewed Oct 23, 2024
View reviewed changes
Copy link
Collaborator

@FilippoZazzeroni FilippoZazzeroni left a comment

Choose a reason for hiding this comment

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

Looks great 🎉 . Nit things i would change the handle methods name to be (e.g)handleToolbarNumberOfTabsChanged to handleNumberOfTabsChangedAction and also Rebase is needed since i noticed some new actions were added.

@ionixjunior
Extract the toolbar did load toolbars to a new function
8142ff9
@ionixjunior
Use the new function to handle toolbar did load toolbars
7ecb97b
@ionixjunior
Extract the toolbar number of tabs changed to a new function
980238b
@ionixjunior
Use the new function to handle toolbar number of tabs changed
fe3bee2
@ionixjunior
Extract the toolbar reader mode state changed to a new function
c2f1a6e
@ionixjunior
Use the new function to handle toolbar reader mode state changed
8146f2e
@ionixjunior
Extract the toolbar website loading state did changed to a new function
568dbc2
@ionixjunior
Use the new function to handle toolbar website loading state did change
019c4d5
@ionixjunior
Extract the toolbar url did change to a new function
46490d6
@ionixjunior
Use the new function to handle toolbar url did change
2f59f75
@ionixjunior
Extract the toolbar back forward button state changed to a new function
013a8db
@ionixjunior
Use the new function to handle toolbar back forward button state changed
dc1f904
@ionixjunior
Extract the toolbar trait collection did change to a new function
a0dd07c
@ionixjunior
Use the new function to handle toolbar trait collection did change
888b80c
@ionixjunior
Extract the toolbar show menu warning badge to a new function
db91a2a
@ionixjunior
Use the new function to handle toolbar show menu warning badge
4f50877
@ionixjunior
Extract the toolbar position changed to a new function
2698dd6
@ionixjunior
Use the new function to handle toolbar position changed
2bab5fe
@ionixjunior
Extract the toolbar did paste search terms to a new function
60e9673
@ionixjunior
Use the new function to handle toolbar did paste search terms
fb1ee1e
@ionixjunior
Extract the toolbar did start editing url to a new function
5b0d526
@ionixjunior
Use the new function to handle toolbar did start editing url
b6c8902
@ionixjunior
Extract the toolbar cancel edit to a new function
ac354ea
@ionixjunior
Use the new function to handle toolbar cancel edit
3dc7575
@ionixjunior
Return the current instance of "state"
3d2cd5d
@ionixjunior
Extract the did delete search term to a new function
385145c
@ionixjunior
Use the new function to handle did delete search term
ce531a6
@ionixjunior
Extract the did enter search term to a new function
4908501
@ionixjunior
Use the new function to handle did enter search term
7455bc3
@ionixjunior
Extract the did start typing to a new function
38ff651
@ionixjunior
Use the new function to handle did start typing
abbe469
@ionixjunior
Rename function
0e1ba2b
@ionixjunior
Rename function
8a5527c
@ionixjunior
Rename function
58d85da
@ionixjunior
Rename function
5a3cafa
@ionixjunior
Rename function
1902ff4
@ionixjunior
Rename function
a14f624
@ionixjunior
Rename function
32af7d6
@ionixjunior
Rename function
c721563
@ionixjunior
Rename function
06726b2
@ionixjunior
Rename function
32f212a
@ionixjunior
Rename function
89678f7
@ionixjunior
Rename function
58a738a
@ionixjunior
Rename function
7bab920
@ionixjunior
Rename function
0dcd99c
@ionixjunior
Rename function
4286641
@ionixjunior
Rename function
4ced2a6
@ionixjunior
Rename function
d8781a4
@ionixjunior
Rename function
bc32500
@ionixjunior ionixjunior changed the title Refactor FXIOS-7301 - Reduce from 272 to 36 lines a property of AddressBarState.swift - help in closure_body_length Refactor FXIOS-7301 - Reduce from 390 to 42 lines a property of AddressBarState.swift - help in closure_body_length Oct 23, 2024
@ionixjunior
Copy link
Contributor Author

Sure, it makes sense. I'll update this in this another PR #22679. Updated and rebased.

@mergify Mergify
Copy link
Contributor

mergify bot commented Oct 24, 2024

This pull request has conflicts when rebasing. Could you fix it @ionixjunior? 🙏

FilippoZazzeroni
FilippoZazzeroni approved these changes Oct 24, 2024
View reviewed changes
Copy link
Collaborator

@FilippoZazzeroni FilippoZazzeroni left a comment

Choose a reason for hiding this comment

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

🎉 super. There is only needed a rebase and we can merge it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FilippoZazzeroni FilippoZazzeroni FilippoZazzeroni approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ionixjunior @FilippoZazzeroni