-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Add Underline in Account Header #2847
Add Underline in Account Header #2847
Conversation
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Bundle Stats — desktop-clientHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger
Smaller No assets were smaller Unchanged
|
Bundle Stats — loot-coreHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset No files were changed View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger No assets were bigger Smaller No assets were smaller Unchanged
|
Hrm, the lines might sit better with a few tweaks, as they are they're a bit dominating. The lines seems a bit too bold - note lines elsewhere are fairly faint. Since they aren't sections of the sidebar, but rather subsections of all accounts, it might make more sense for them to not go fully across and instead only start where the text does, and end at the end of the numbers. |
Thinner line is good, thought I'd mock up my poorly worded other suggestion about line width, please forgive my bad paint job skills. I think the second does look better and gives a better sense of hierarchy than all the way across since there's already a fully across line break between the menu options and the accounts section. |
I agree that the second is more desireable. With lines all the way across it looks like entirely new sections of the menu rather than a subsection of accounts. While it may be simpler to code the first, the second is doable and I think we should make an effort to use that look instead. |
…1099/actual into accountNavVisualChanges
Im not sure what you are referring to by this. It looks good to me, pretty much exactly what I think @carkom was asking for. |
Yeah your right. I was thinking when selected it should highlight purple, but honestly more I look at it, the more I think not. I'm happy with getting this merged. |
Looks good to me, thanks for taking the feedback on board @Crazypkr1099 :) |
Can we merge it then? |
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.
Found this minor problem in the master branch after updating my branches to master
style={ | ||
(name === 'Off budget' || name === 'For budget') && { | ||
borderBottom: `1.5px solid rgba(255,255,255,0.4)`, | ||
paddingBottom: '3px;', |
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.
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.
Ah crap sorry... Didn't know about this. I'll create a PR tonight to fix this.
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.
No description provided.