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

PETAL #81

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

PETAL #81

wants to merge 3 commits into from

Conversation

btkostner
Copy link
Contributor

This is more of a fun PR

  • Adds Live View
  • Adds alpinejs
  • Updates the 2fa setup page to use live view

TODO:

  • Add tailwind
  • Add System76 omnibar, header, and footer

@btkostner btkostner requested a review from a team as a code owner January 29, 2021 20:02
} else {
document.addEventListener('DOMContentLoaded', fn)
const csrfToken = document.querySelector('meta[name="csrf-token"]').getAttribute('content')
const liveSocket = new LiveSocket('/live', Socket, {
Copy link
Contributor

Choose a reason for hiding this comment

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

also need the dom key here so alpine components don't lose their state when liveview patches the dom. docs

  dom: {
    onBeforeElUpdated(from, to) {
      if (from.__x) {
        window.Alpine.clone(from.__x, to)
      }
    }
  },

}
topbar.config({barColors: {0: '#63b1bc'}, shadowBlur: 0})
window.addEventListener('phx:page-loading-start', info => topbar.show())
window.addEventListener('phx:page-loading-stop', info => topbar.hide())
Copy link
Contributor

Choose a reason for hiding this comment

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

wdyt of adding a delay before showing the loading bar? the loading bar could be distracting if it shows and 50ms later clears; so we could only show it if the request is taking longer than (eg) 200ms.

let topbarDelay = null;

window.addEventListener("phx:page-loading-start", _info => {
  clearTimeout(topbarDelay);
  topbarDelay = setTimeout(() => topbar.show(), 200);
})

window.addEventListener("phx:page-loading-stop", _info => {
  clearTimeout(topbarDelay);
  topbar.hide();
})

Organization Account
<% end %>
</div>
</div>

<div class="field company_name" style="display:<%= business_type_class(@changeset) %>;">
<div class="field" x-show="accountType === 'business'" style="display:<%= business_type_class(@changeset) %>;">
Copy link
Contributor

Choose a reason for hiding this comment

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

alternative: with AlpineJS you could add an x-cloak attribute and some css [x-cloak] { display: none }. AlpineJS will have x-show and x-cloak work together to prevent the flash of uninitialized components. This way you don't have to manage it with a view function to conditionally add this class yourself.

<%= get_flash(@conn, :error) %>
</div>
<% end %>
<%= if get_flash(@conn, :info) do %>
Copy link
Contributor

Choose a reason for hiding this comment

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

instead of refetching in the body, you could assign at the same time, if info = get_flash(@conn, :info) do

@@ -96,7 +97,6 @@ defmodule RecognizerWeb.Router do

get "/settings", UserSettingsController, :edit
put "/settings", UserSettingsController, :update
get "/settings/two-factor", UserSettingsController, :two_factor
post "/settings/two-factor", UserSettingsController, :two_factor_confirm
live "/settings/two-factor", TwoFactorSettingsLive
Copy link
Contributor

Choose a reason for hiding this comment

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

heads up, you could specify the :index action here, which gives you Routes.two_factor_settings_path(@conn, :index). I found this more consistent with other path helpers instead of all liveview paths going through live_path.

it's a little awkward though because there's no real :index action; we're just informing how to create the route helper.

@macifell macifell marked this pull request as draft November 3, 2022 17:51
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

2 participants