- Notifications
You must be signed in to change notification settings - Fork584
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
feat: allow choosing favorite buttons in bottom navigation bar#2761
feat: allow choosing favorite buttons in bottom navigation bar#2761
Conversation
|
✅ Deploy Preview forelk-docs canceled.
|
✅ Deploy Preview forelk-zone ready!
To edit notification comments on pull requests, go to yourNetlify site configuration. |
4fb121e tof88bd29Compare67fa668 tofaf253cComparefaf253c to7f8a2a1Compare| <button | ||
| v-for="{ name, label, icon } in availableNavButtons" | ||
| :key="name" | ||
| btn-text flex="~ gap-2" p2 border="~ base rounded" bg-base ws-nowrap |
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.
maybe you can addselected option toNavButton to simply logic...
can you add these attributes?
type="button" role="switch" :aria-checked="selectedNavButtonNames.includes(name)"
and removedisable attribute?
You also need to include toggle logic, adding some style instead disabling it
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'll check it next weekend on my local, don't apply changes yet
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.
Thanks for suggestion for button styling!
I'm actually still explorering what kind of UI would be better overall. A bit more adjustment would be included until finalizing. Maybe I'll add some ordering button or better remove button etc. later for the easy arrangement.
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.
Maybe an editable forn? Check push notification in notifications settings (you will needd to enable them, dont forget to remove once you've check it)
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.
OK, I was able to implement the toggle button logic now. 🙂
9d3743a to6ca5557Compare7216716 toa7ce23dCompareNow it's ready to review.
|
a7ce23d tocda518cComparecda518c tobf1fcb1CompareThere 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.
This is incredible! A lot of people will be very happy when this feature is released.
The current way to show what buttons were available wasn't very clear to me, and I didn't discover easily that I should first clear then add the buttons to get the order I want. I don't have a better proposal that doesn't involve complex drag and drop so I think we should merge this for now. Users aren't going to use this all the time. If we have a better idea later, we can improve it.
@patak-dev I just realized that in the normal use case, users want to create a different button arrangement than the current one so there is no need to place them as default. I made#2803 to set the default selection empty. Yes, the drag and drop pattern was exactly one of the candidates I was thinking about, but avoid it for now due to complexity for now. If I come up with a better UI, I'll make another PR. |
resolves#2757