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

feat: Character tab improvements #425

Merged
merged 5 commits into from
Jun 20, 2024

Conversation

FI00ds
Copy link
Contributor

@FI00ds FI00ds commented Jun 11, 2024

Pull Request

Description

  • Added a searchbar and element/path filters to the top of the character tab

Related Issue

Checklist

  • I have added commit messages that are descriptive and meaningful.
  • I have tested the changes locally.
  • I have reviewed the code changes.

Screenshots

Before

firefox_2024-06-11_17-11-04

After

firefox_2024-06-11_17-09-31

@fribbels
Copy link
Owner

The element/path filters are a nice touch, works great.

gridRef.current.api.onFilterChanged()
}, [])

const charinfo = JSON.parse(JSON.stringify(DB.getMetadata().characters))
Copy link
Owner

Choose a reason for hiding this comment

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

This probably shouldn't be outside the callback, we don't need this on every render. Should be a Utils.clone


const doesExternalFilterPass = useCallback(
(node) => {
if (currentFilters.element.length && !currentFilters.element.includes(charinfo[node.data.id].element)) {
Copy link
Owner

Choose a reason for hiding this comment

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

A small readability improvement here, instead of charinfo[node.data.id] everywhere we can name the variable as filteredCharacter

    const characterMetadata = Utils.clone(DB.getMetadata().characters)
    const filteredCharacter = characterMetadata[node.data.id]

    ... currentFilters.element.includes(filteredCharacter.element) ...

return false
}
return true
}, [charinfo, currentFilters],
Copy link
Owner

Choose a reason for hiding this comment

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

Don't need charinfo as a dependency because the full cast of characters never changes

Comment on lines 250 to 253
if (!((charinfo[node.data.id].name).toLowerCase().includes(currentFilters.name) || (charinfo[node.data.id].displayName).toLowerCase().includes(currentFilters.name))) {
return false
}
return true
Copy link
Owner

Choose a reason for hiding this comment

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

Can probably Demorgans law this into something simpler, remove the negation by returning the inverse

    return (filteredCharacter.name).toLowerCase().includes(currentFilters.name)
      || (filteredCharacter.displayName).toLowerCase().includes(currentFilters.name)

Comment on lines 257 to 260
const isExternalFilterPresent = useCallback(() => {
if (!currentFilters.element.length && !currentFilters.path.length && !currentFilters.name.length) return false
return true
}, [currentFilters])
Copy link
Owner

Choose a reason for hiding this comment

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

There's a very high performance impact of using [currentFilters] as a dependency here because this rerenders the entire character tab on every search bar keystroke and I'm getting some noticeable lag when typing in the bar. Probably needs an overall refactor to get this modularized enough so it doesn't do that. I can give it a shot unless you'd prefer to.

@@ -222,6 +232,33 @@ export default function CharacterTab() {
cellStyle: { display: 'flex' },
}), [])

const externalFilterChanged = useCallback((newValue) => {
setCurrentFilters(newValue)
gridRef.current.api.onFilterChanged()
Copy link
Owner

Choose a reason for hiding this comment

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

I think gridRef doesnt actually exist here, needs to be characterGrid


const doesExternalFilterPass = useCallback(
(node) => {
const charinfo = Utils.clone(DB.getMetadata().characters)
Copy link
Owner

Choose a reason for hiding this comment

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

After some profiling, I found most of the lag was coming from this.

This clone isn't necessary here since no modifications are made, and DB.getMetadata().characters is a massive json, so its triggering a lag spike by cloning it on every keystroke

Comment on lines 178 to 180
const [cardfilters, setCardfilters] = useState(Utils.clone(defaultFilters))

const namefilter = useRef('')
Copy link
Owner

Choose a reason for hiding this comment

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

Camel case naming reminder

Comment on lines 517 to 519
let newFilters = Utils.clone(namefilter.current)
newFilters = e.target.value.toLowerCase()
namefilter.current = newFilters
Copy link
Owner

Choose a reason for hiding this comment

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

I think all this can just be simplified to:

nameFilter.current = e.target.value.toLowerCase()

@fribbels fribbels force-pushed the feat/character-tab-improvements branch from 73386b6 to 7c53c2e Compare June 20, 2024 02:49
@fribbels fribbels merged commit 93baac9 into fribbels:beta Jun 20, 2024
1 check passed
fribbels added a commit that referenced this pull request Jul 9, 2024
* feat: add relic api replacement (#446)

* hotfix: new relic scorer api fix

* hotfix: revert api changes

* Added All and None buttons to character select screen. (#445)

* Added All and None buttons to character select screen.

* feat: adjusting button text, styling, setting CUSTOM as the default on relics tab graph

---------

Co-authored-by: WolfBungalow <aastir@gmail.com>
Co-authored-by: Fribbels <fribbelsgithub@gmail.com>

* chore: update Node.js and official GitHub Actions version (#438)

* chore: update Node.js and official GitHub Actions version

* chore: update package-lock

---------

Co-authored-by: Fribbels <fribbelsgithub@gmail.com>

* feat: Character tab improvements (#425)

* feat: header with filtering + search bar

* fix: searchbar performance
chore: readability

* condensed filter logic

* feat: fixing lag spikes, refactor/rename the filters, some styling adjustments for grids/filters

* fix: cleanup dead code

---------

Co-authored-by: Fribbels <fribbelsgithub@gmail.com>

* hotfix: accidentally deleted a placeholder div

* feat: recenter light cones, remove optimizer weight column, fix some styling (#449)

* feat: character tab styling

* feat: re-adjust light cone offsets

* fix: min width for character select modal

* refactor: remove the rest of optimizer weight column remnants

* feature: new logic for ability type buffs (#458)

* feat: adding ability type buff conditional handling

* feat: adding dmg boost types initial commit, renaming

* fix: herta skill hp50% buff is now correctly 20%

* fix: sushang's Sword Stance trace is now correctly 2% dmg

* draft

* feat: finished migrating all the ability buffs

* refactor: partial refactor to bit flags

* feat: finish the bitwise refactor

* docs: update comments

* fix: updating optimizer grid col

* fix: sushang's trace is actually 0.025%

* feat: scoring buff for faster diminishing returns on 100% benchmark (#460)

* feat: beta v3 changes (#465)

* feat: beta v3 changes

* feat: update Block -> Parry

* feature: final changes for 07-08 update + changelog (#468)

* feat: updating optimizer column header styling, add weight back

* feat: update changelog, add ashblazing calcs for yunli march, disable seele e1, update yunli teams

* feat: jiaoqiu v4 changes

---------

Co-authored-by: WolfBungalow <irtwit@gmail.com>
Co-authored-by: WolfBungalow <aastir@gmail.com>
Co-authored-by: Garfield Lee <Garfield550@users.noreply.github.com>
Co-authored-by: FI00ds <122728302+FI00ds@users.noreply.github.com>
@FI00ds FI00ds deleted the feat/character-tab-improvements branch July 9, 2024 08:39
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.

None yet

2 participants