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

Minor: simplification of example. #1018

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ignatz
Copy link

@ignatz ignatz commented Jan 30, 2024

I'm new to fluent_ui and as I was reading through the example I found it to be slightly harder to follow than necessary. These are my suggestions to streamline the initial exposure. Nothing much of substance.

EDIT: I also took the liberty to include footer items into the auto suggestions.

Pre-launch Checklist

  • I have updated CHANGELOG.md with my changes
  • I have run "dart format ." on the project
  • I have added/updated relevant documentation

I'm new to fluent_ui and as I was reading through the example I found it to be slightly harder to follow than necessary. These are my suggestions to streamline the initial exposure.
@hunt978
Copy link

hunt978 commented Feb 4, 2024

It took me three days to figure out how the home page works. The main issue with this demonstration is the duplicated information required by the go_router and pane_items. One possible solution is grouping navgation prompting, routing and page building together. By supplying builders for either go_router or pane_item, the reduction of total complexity could have been archive.

@ignatz
Copy link
Author

ignatz commented Feb 4, 2024

Hey @hunt978 , I did notice and appreciate that part too. That said, I didn't dare to change that part. I wasn't sure if that was maybe intentional to demonstrate the use of gorouter as a popular package 🤷

Comment on lines -516 to -523
paneBodyBuilder: (item, child) {
final name =
item?.key is ValueKey ? (item!.key as ValueKey).value : null;
return FocusTraversalGroup(
key: ValueKey('body$name'),
child: widget.child,
);
},
Copy link
Owner

Choose a reason for hiding this comment

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

Why was this removed? This is important to keep the focus on the page.

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

3 participants