Skip to content

Conversation

@avilagaston9
Copy link
Contributor

@avilagaston9 avilagaston9 commented Dec 18, 2024

Highlight Current Page

Description

Highlights the current page in the nav bar in the explorer.

image
image

Type of change

  • New feature

Checklist

  • “Hotfix” to testnet, everything else to staging
  • Linked to Github Issue
  • This change depends on code or research by an external entity
    • Acknowledgements were updated to give credit
  • Unit tests added
  • This change requires new documentation.
    • Documentation has been added/updated.
  • This change is an Optimization
    • Benchmarks added/run
  • Has a known issue
  • If your PR changes the Operator compatibility (Ex: Upgrade prover versions)
    • This PR adds compatibility for operator for both versions and do not change batcher/docs/examples
    • This PR updates batcher and docs/examples to the newer version. This requires the operator are already updated to be compatible

@avilagaston9 avilagaston9 self-assigned this Dec 18, 2024
@avilagaston9 avilagaston9 marked this pull request as ready for review December 18, 2024 13:19
"transform",
"duration-150",
"active:scale-95",
active_view_class(assigns.socket.view, ExplorerWeb.Home.Index)
Copy link
Member

@MarcosNicolau MarcosNicolau Dec 18, 2024

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.

Copy link
Contributor

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

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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

Comment on lines 126 to 135
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"
Copy link
Member

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.

Copy link
Contributor

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)

Copy link
Contributor

@Oppen Oppen Dec 18, 2024

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"

Copy link
Contributor

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"

Copy link
Contributor Author

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.

Copy link
Contributor Author

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

Copy link
Contributor

@uri-99 uri-99 left a comment

Choose a reason for hiding this comment

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

left some comments

@uri-99 uri-99 changed the base branch from staging to explorer-new-version December 19, 2024 16:29
@uri-99 uri-99 merged commit 9b10fa4 into explorer-new-version Dec 19, 2024
1 check passed
@uri-99 uri-99 deleted the highlight-current-page branch December 19, 2024 16:43
@uri-99 uri-99 mentioned this pull request Dec 19, 2024
17 tasks
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.

5 participants