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

Trending Tokens UI implementation phase 1 #6276

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

greg-schrammel
Copy link
Contributor

@greg-schrammel greg-schrammel commented Nov 21, 2024

Fixes APP-1957
Fixes APP-1958

What changed (plus any additional context for devs)

Screen.Recording.2024-11-25.at.14.50.21.mov

I couldn't make the Drag N Drop we already have work, but don't like this implementation, it forces rerenders in the js thread, I think a better solution is to absolutely position everything and track it all in shared values

  • what should we do if user select a network that's in the expanded state? made it jump to other grid to be visible but idk
  • the yellow with the star in the New filter is not good

What should we show when user pins all networks? like a placeholder to drag into to unpin

Screenshot 2024-11-25 at 14 52 05

ops forgot to enable this in the recording, it won't show again for 2 weeks after dismissed
it's not straight forward to add a border like in the designs, so ignored it for now but doable if we feel like it

Screenshot 2024-11-25 at 15 05 43

Screen recordings / screenshots

What to test

Copy link

linear bot commented Nov 21, 2024

Copy link

linear bot commented Nov 25, 2024

@@ -39,5 +39,6 @@ module.exports = {
],
'jest/expect-expect': 'off',
'jest/no-disabled-tests': 'off',
'no-nested-ternary': 'off',
Copy link
Contributor

Choose a reason for hiding this comment

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

good

@@ -49,7 +49,7 @@ const darkModeColors = {
skeleton: '#191B21',
stackBackground: '#000000',
surfacePrimary: '#000000',
white: '#12131A',
white: '#000',
Copy link
Contributor

Choose a reason for hiding this comment

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

hesitant to do this, can we make a new color instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

can you add remote config as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

let's split these components up into separate files pls

Copy link
Contributor

Choose a reason for hiding this comment

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

same here (split up)

@derHowie
Copy link
Member

hey @greg-schrammel, what should I test for here do you think?

Comment on lines +532 to +535
const toggleSelected = (chainId: ChainId) => {
if (selected.includes(chainId)) unselect(chainId);
else select(chainId);
};
Copy link
Contributor

Choose a reason for hiding this comment

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

From what I understand of the spec, we only want to be able to select one network at a time (or all networks). This allows multiple to be selected.

Copy link
Contributor

@walmat walmat left a comment

Choose a reason for hiding this comment

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

only thing blocking merge at this point is network selection being limited to one network and storing that in the zustand store to display on the dropdown. Otherwise lgtm

Comment on lines +745 to +754
const unselect = (chainId: ChainId) =>
setSelected(s => {
if (s === 'all') return [];
return s.filter(id => id !== chainId);
});
const select = (chainId: ChainId) =>
setSelected(s => {
if (s === 'all') return [chainId];
return multiple ? [...s, chainId] : [chainId];
});
Copy link
Contributor

Choose a reason for hiding this comment

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

From what I understand of the spec, we only want to be able to select one network at a time (or all networks). This allows multiple to be selected.

This is probably more relevant here actually

transform: [{ translateY: translationY.value }],
}));

const [selected, setSelected] = useState<ChainId[] | 'all'>([]);
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be stored in the zustand store?

Copy link
Contributor

Choose a reason for hiding this comment

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

I actually think it makes sense to move a lot of the internal helpers (selection, editing, etc.) to the Zustand store but that's my personal preference

@walmat
Copy link
Contributor

walmat commented Nov 27, 2024

Screenshot 2024-11-27 at 12 39 37 PM

Another issue, let's wrap those pills in a horizontal scrollview in case it's needed

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.

3 participants