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

Make AdminLessonNav generic so it can be reused #2268

Closed
bryanjenningz opened this issue Sep 8, 2022 · 0 comments · Fixed by #2272
Closed

Make AdminLessonNav generic so it can be reused #2268

bryanjenningz opened this issue Sep 8, 2022 · 0 comments · Fixed by #2272
Assignees

Comments

@bryanjenningz
Copy link
Collaborator

bryanjenningz commented Sep 8, 2022

AdminLessonNav is currently only used in the admin page, but we want to have the same style of navigation tabs in the DOJO exercises page, so it makes sense to make AdminLessonNav more generic and use it in both places since both places have the same requirements and behavior and we want them to be consistent with each other.

We also want the API to require less boilerplate to use. Currently, we need to pass in our own component which requires some boilerplate.

Since the only parts of the nav tabs that change are the nav tab text and the url and whether a tab is selected, we can change the AdminLessonNav props so it just takes in the props { tabs: { text: string, url: string, isSelected: boolean }[] } to represent each tab. We can expect the styling of a selected and unselected tab to be consistent for each use case, so these simplified props cover all our use cases and it requires less boilerplate than the current props which require you to pass in a component.

Problems with this approach:

  • It's possible for multiple tabs to be selected, which seems like an invalid state, we could solve this by instead having the props be { selectedTabIndex: number | null, tabs: { text: string, url: string }[] }.
  • We could potentially remove the isSelected and selectedTabIndex and just have the AdminLessonNav determine if a tab is selected by checking the current URL with the URL of each tab. This would make it so we only need to pass in props { tabs: { text: string, url: string }[] }, which is more simple, but it makes it so the tab is only selected if the current URL exactly matches the tab.
  • We don't have a high degree of customization of the styling, we could solve this by adding an optional component that we can pass in which determines the look of each tab, similar to the current props but this time it's optional.
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 a pull request may close this issue.

1 participant