Skip to content

Conversation

@joedixon
Copy link
Contributor

@joedixon joedixon commented Dec 10, 2020

I looked through the settings pages of Tailwind UI and like the layout I have opted for. This did mean merging the profile and the password settings onto one page (a separate form and panel for each).

Screenshot 2020-12-10 at 15 01 00

Screenshot 2020-12-10 at 15 03 05

Screenshot 2020-12-10 at 15 02 54

Screenshot 2020-12-10 at 15 02 00

Screenshot 2020-12-10 at 15 01 54

Screenshot 2020-12-10 at 15 01 08

Screenshot 2020-12-10 at 15 03 46

Also resolves #576

@driesvints
Copy link
Member

This looks super good! But I'm thinking we can ditch the menu on the left entirely because it's already present in the navigation dropdown. I don't feel it serves much purpose there.

Sort of like this screen here: https://tailwindui.com/components/application-ui/page-examples/settings-screens#component-c29fe3f9f780c97a58f08453fc6bbec8

@joedixon
Copy link
Contributor Author

This looks super good! But I'm thinking we can ditch the menu on the left entirely because it's already present in the navigation dropdown. I don't feel it serves much purpose there.

Sort of like this screen here: https://tailwindui.com/components/application-ui/page-examples/settings-screens#component-c29fe3f9f780c97a58f08453fc6bbec8

My thinking was that as we add more user features, the navbar will get a bit bloated. I liked the idea of having a single navbar item that takes you do the user area and you can nvaigate your way from there.

I'm happyt to be talked around though 😄

@driesvints
Copy link
Member

It just feels really out of place to me. Especially since the menu will disappear if you click any item except the settings. The navbar menu just has a handful of items and isn't bloated at all I think. Now that we have a single page for all our user settings I think it makes sense to just have the page centered without a menu next to it. If we have lots of extra settings in the future we can revise of course.

Screenshot 2020-12-11 at 15 41 28

@joedixon
Copy link
Contributor Author

It just feels really out of place to me. Especially since the menu will disappear if you click any item except the settings. The navbar menu just has a handful of items and isn't bloated at all I think. Now that we have a single page for all our user settings I think it makes sense to just have the page centered without a menu next to it. If we have lots of extra settings in the future we can revise of course.

My plan was to make the sidebar persistent across all user pages so in the long-run, it wouldn't disappear. I'm happy to park that idea for now and go with the single centered column. Not sure when I'll be able to get around to it, but will try and find some time next week.

@driesvints
Copy link
Member

How would that look for the profile page though? What happens for the profile page when you're not logged in?

I think it's best to remove it for now and revisit it when we actually need it.

@joedixon
Copy link
Contributor Author

Settings screenshot

I removed the sidebar and made the panels centered on the page. I also added the ability to have a prefix icon in the form input.

@driesvints
Copy link
Member

@joedixon cool. Tests are failing it seems.

@joedixon
Copy link
Contributor Author

@joedixon cool. Tests are failing it seems.

That will teach me to change the button text. Should be good now.

@joedixon joedixon requested a review from driesvints December 28, 2020 15:53
Copy link
Member

@driesvints driesvints left a comment

Choose a reason for hiding this comment

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

Nice job. I left some remarks.

A few notes in general:

  • Always try to leave some breathing room before or after an if directive in Blade views when there's elements on the same level before or after it.
  • If directives always have a space between if and the opening (
  • I see a lot of {{ syntax being used in Blade component value attributes. Did you test all of these? Because afaik this isn't supported on Blade components. You should try to use the :value="Auth::user()->email" syntax instead.

@driesvints
Copy link
Member

@joedixon tests seem to be failing.

@joedixon
Copy link
Contributor Author

joedixon commented Jan 8, 2021

@joedixon tests seem to be failing.

Fixed!

@driesvints driesvints merged commit 5f37506 into main Jan 8, 2021
@driesvints driesvints deleted the settings branch January 8, 2021 10:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Inform Twitter handles should not include @

3 participants