Skip to content
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

WIP: User Brew Theme Write-in #3923

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

dbolack-ab
Copy link
Collaborator

Description

A first pass at adding a write-in option for User Brews. Functional, but fugly. Provided for feedback.

Needs:

  • opinions on UI placement
  • opinions on best choice for displaying a write-in based User Brew ( flip to write-in box? Add to drop-down list? )
  • Sanity checking? on brew share-id input.
  • Should it only try to load after enter/cr?

Related Issues or Discussions

  • Closes #

QA Instructions, Screenshots, Recordings

Reviewer Checklist

Please replace the list below with specific features you want reviewers to look at.

*Reviewers, refer to this list when testing features, or suggest new items *

  • Verify new features are functional
    • Hitting arrow toggle at end of the themes list toggles between input modes
    • Inputting a valid share-id changes the brew theming
    • Returning to the Pulldown list and selecting a theme works
  • Verify old features have not broken
  • Test for edge cases / try to break things
    • Invalid shareIds are handled in a useful way for the user.
  • Identify opportunities for simplification and refactoring
  • Check for code legibility and appropriate comments

Needs:

 - [ ] opinions on UI placement
 - [ ] opinions on best choice for displaying a write-in based User Brew ( flip to writin box? Add to drop-down list? )
@Gazook89
Copy link
Collaborator

Just for clarity, here is what it looks like at this time:

image

Note the little arrow icon on the right side of the theme dropdown...clicking that opens the below text input:

image

As stated in the PR the styling/structure isn't complete, but wanted to point this out because it took me a minute to even realize that arrow existed at all (particularly when i opened in Firefox, since the font awesome icons don't render).

@Gazook89
Copy link
Collaborator

Regarding opinions on the actual questions about what this should look like:

What about a dropdown menu option at the bottom that says "User Theme...". Choosing that displays the text input for writing in your own, below the theme select.

If something is already entered in the user theme input, and then they select an included theme, it should hide the user theme input again. But in state, it shouldn't clear that user input quite yet, in case the user decides to go back to using the write-in option again-- they shouldn't have to enter it again. But preserving this value is just a client-side thing...reloading the page wouldn't preserve it.

Does that make sense?

@dbolack-ab
Copy link
Collaborator Author

dbolack-ab commented Dec 1, 2024

As stated in the PR the styling/structure isn't complete, but wanted to point this out because it took me a minute to even realize that arrow existed at all (particularly when i opened in Firefox, since the font awesome icons don't render).

Yeah, that arrow was duplicated from the thumbnail hider and isn't the most obvious with the length of the fake pulldown.

Holding in state makes sense.

@5e-Cleric 5e-Cleric added 🔍 R1 - Reviewed - Needs Discussion 💬 PR not okayed yet; needs reevaluation or closure feature Approved Has been discussed and an approach is agreed upon labels Dec 26, 2024
Still issues with saving the state  of the theme pulldowns and collecting the written in theme.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved Has been discussed and an approach is agreed upon feature 🔍 R1 - Reviewed - Needs Discussion 💬 PR not okayed yet; needs reevaluation or closure
Projects
Status: Pushing to Finish
Development

Successfully merging this pull request may close these issues.

5 participants