-
Notifications
You must be signed in to change notification settings - Fork 391
feat(explorer): highlight current page #1638
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
Conversation
| "transform", | ||
| "duration-150", | ||
| "active:scale-95", | ||
| active_view_class(assigns.socket.view, ExplorerWeb.Home.Index) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would not change the logo size, it looks weird.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 , i would experiment other effects. i.e. making it brighter or underline it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If brighter, I think we'd need better contrast, like a much darker default and much brighter highlight. I think the underline would be easier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will try using the underline. I have tried other effects, but they didn't seem to have any impact, possibly because it is an icon, not text.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the effects on the icon since they have no impact: #39a6ec2
| defp active_view_class(ExplorerWeb.Home.Index, ExplorerWeb.Home.Index), do: "text-4xl" | ||
| defp active_view_class(_other, ExplorerWeb.Home.Index), do: "text-3xl" | ||
|
|
||
| defp active_view_class(ExplorerWeb.Batches.Index, ExplorerWeb.Batches.Index ), do: "text-green-500 font-bold" | ||
| defp active_view_class(ExplorerWeb.Batch.Index, ExplorerWeb.Batches.Index ), do: "text-green-500 font-bold" | ||
| defp active_view_class(_other, ExplorerWeb.Batches.Index ), do: "text-foreground/80 hover:text-foreground font-semibold" | ||
|
|
||
| defp active_view_class(ExplorerWeb.Operators.Index, ExplorerWeb.Operators.Index ), do: "text-green-500 font-bold" | ||
| defp active_view_class(ExplorerWeb.Operator.Index, ExplorerWeb.Operators.Index ), do: "text-green-500 font-bold" | ||
| defp active_view_class(_other, ExplorerWeb.Operators.Index ), do: "text-foreground/80 hover:text-foreground font-semibold" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would avoid pattern matching here. You end up repeating the styles all over and it make testing and updating each case very difficult.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What alternative do you suggest?
I think the only problem with the current solution is that uncatched cases are not identifiable (i.e. the restakings page)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For reducing styling duplicates, maybe just use a separate CSS class and return only that class might work, right? I mean, with the exception of the logo everything follows the pattern "if active make green and bold, otherwise default text-foreground and semibold".
Then, we can trim the cases a bit by considering that the only same-class pair that does not follow this pattern is the logo, so everything else could be covered by a single pattern:
defp active_view_class(class_x, class_x), do: "active-view"Similarly, our fallthrough to non-highlighter would be:
defp active_view_class(class_x, class_y), do: "inactive-view"There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the end I think we would have something like this, if applying this idea:
defp active_view_class(ExplorerWeb.Home.Index, ExplorerWeb.Home.Index), do: "active-view aligned-icon"
defp active_view_class(_other, ExplorerWeb.Home.Index), do: "inactive-view aligned-icon"
defp active_view_class(class_x, class_x), do: "active-view"
defp active_view_class(ExplorerWeb.Operator.Index, ExplorerWeb.Operators.Index), do: "active-view"
defp active_view_class(ExplorerWeb.Batch.Index, ExplorerWeb.Batches.Index), do: "active-view"
defp active_view_class(class_x, class_y), do: "inactive-view"There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How much do we actually gain with this? I don't find the last solution significantly clearer than the current one. Also, we are only reducing two lines, but now we need to add four new class definitions. That said, I'm open to changing it if you think it's worthwhile.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since I removed the effects on the icon, I was able to simplify the code a bit: #39a6ec2
uri-99
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left some comments
Highlight Current Page
Description
Highlights the current page in the nav bar in the explorer.
Type of change
Checklist
testnet, everything else tostaging