-
Notifications
You must be signed in to change notification settings - Fork 3
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
base: master
Are you sure you want to change the base?
PETAL #81
Conversation
} else { | ||
document.addEventListener('DOMContentLoaded', fn) | ||
const csrfToken = document.querySelector('meta[name="csrf-token"]').getAttribute('content') | ||
const liveSocket = new LiveSocket('/live', Socket, { |
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.
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()) |
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.
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) %>;"> |
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.
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 %> |
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.
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 |
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.
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.
This is more of a fun PR
TODO: