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

UI small fixes #2266

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

oliveirarleo
Copy link

@oliveirarleo oliveirarleo commented Jul 8, 2024

Hi guys, sorry for the delay, I ended up going on a trip as I am in my vacation.

Validation Steps

  • Simply open the pages for workflow, history and settings page, the projects menu will be hidden when there is no project assigned yet.
  • In the workflow job editor page, there is no flickering.
    reference for the original bug https://www.loom.com/share/58feb3af170a42fdbe70a127843bb7cb
  • The save button still blinks, but does not change size, the user does not have time to read still.
  • Some texts are standardized with ... for the phx-disable-with text.
  • It's unclear if other buttons are flickering, I am new to this project, and did not find other pages with the same problems, but reading the code they should exist.

Notes for the reviewer

At the lib/lightning_web/live/components/menu.ex, github fails to diff in a good way. You can notice that the previous code is the same as in the else block, this should simplify the review.

Related issue

Fixes #2212

Review checklist

  • I have performed a self-review of my code
  • I have verified that all appropriate authorization policies (:owner, :admin, :editor, :viewer) have been implemented and tested
  • If needed, I have updated the changelog
  • Product has QA'd this feature

@oliveirarleo oliveirarleo marked this pull request as ready for review July 13, 2024 14:51
<button
type="button"
class="relative w-full cursor-default rounded-md bg-white py-1.5 pl-3
<%= if length(assigns[:projects]) == 0 do %>
Copy link
Member

Choose a reason for hiding this comment

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

When accessing assigns in Heex templates, please use @projects - that way the renderer can optimise for diffs/prevent rerenderers.

The example below, assigns[:selected_project] is an exception to the rule since in that specific case we don't know if there is an assign with that key.

But for any other case, we always use the @name style.

Copy link

@christad92 christad92 left a comment

Choose a reason for hiding this comment

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

With fixed width implemented for the save button, the text is small so we have too much space on both sides of the "Save" CTA. See image below:
image

My recommendation is to set the CTA to Save edit. This will reduce the padding and remove the flashes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In review
Development

Successfully merging this pull request may close these issues.

Standardize button styles and behaviour
3 participants