-
-
Notifications
You must be signed in to change notification settings - Fork 64
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
New tabbed edit package UI #410
Conversation
Thanks for your pull request, @WebFreak001! |
imo there should be a way to dismiss warnings because there is no way to fix them anyway for an already released version |
br | ||
| #[strong= stats.monthly.to!string] downloads this month | ||
br | ||
| #[strong= stats.total.to!string] downloads total |
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.
I guess this could be a separate tab "statistics"?
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.
I quite like having the statistics directly when you open the package instead.
Maybe a more dedicated border and some more stats would make this better.
public/scripts/pkgadmin.js
Outdated
|
||
var links = subtabs.querySelectorAll("a.tab"); | ||
|
||
if (openpage) |
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.
openpage vs. hasPage below -> be consistent in naming.
public/scripts/pkgadmin.js
Outdated
hasPage = true; | ||
if (!hasPage) | ||
return; | ||
} |
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.
Move into separate function
public/scripts/pkgadmin.js
Outdated
upgradeSubtabs(subtabs[i], openpage); | ||
} | ||
|
||
upgradeAllSubtabs(""); |
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.
Be consistent. The call below uses no arguments.
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.
passing empty string initializes with default page, passing with no args will put in the current hash
Looking at it this is a very nice improvement 👍, but I think we should try to make the "General" tab a bit prettier. It looks very unstructured at the moment: Also, now that we have tabs I guess we could add "Badges" as another one and show sth. similar to Maybe in a style similar to: But I will open a separate issue for this. (-> #411) |
Have you tried moving the logo to the right side and inserting a border in-between? |
Alternatively if this still looks weird, maybe the logo can always be a separate tab (as its not too often updated)? |
I think it's wasted space moving that into a separate tab (and like this it directly shows you an image of what you are editing), so I would definitely keep it in the general tab. I will add a small separator line |
anything else for this PR? It's fairly trivial to add new tabs (just a button and a new div with anchor like the others) so anything additional could also be added later. I think as for how this is right now, this is a fairly well working solution. |
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.
Looks good to me, but I didn't had time to dive into all CSS changes, so I would be happy if someone else could have a look at this too. @thewilsonator maybe?
(preview can be done with "View deployment" and the user |
I'm really not good with web stuff, @ZombineDev? |
Ping. Could anyone spare a second pairs of eyes here? |
I was not able to login with those credentials. |
Ah. The registry seems to auto-lowercase the username on save, but not when checking for the login. Try |
This makes it easier to extend the package configuration and add more settings without cluttering the UI
Tip for reviewing: disable whitespace in diff options
using tabs (if JS is enabled)
Screenshots:
Desktop view
Mobile view
no-JS view (kind of like before, but with buttons at the top to jump around)
also package view looks slighly different now to better highlight which tab is selected: