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

Project Proposal: Core UI improvement #912

Merged
merged 6 commits into from
Mar 23, 2023

Conversation

fcoveram
Copy link
Contributor

@fcoveram fcoveram commented Mar 14, 2023

Project proposal for #415

Due date

Review for this proposal should aim to be completed by 28 March 2023.

Notes

I added @zackkrida and @dhruvkb as reviewers. There are a few sections with no information as it might require some technical scope. I would love a special focus on the "Infrastructure" and "Success" sections as the former needs to assess the impact of this work, to then set the success criteria for the latter section.

Since this is my first project proposal document, feel free to suggest any other change.

Checklist

  • My pull request has a descriptive title (not a vague title like
    Update index.md).
  • My pull request targets the default branch of the repository (main) or
    a parent feature branch.
  • My commit messages follow best practices.
  • My code follows the established code style of the repository.
  • [N/A] I added or updated tests for the changes I made (if applicable).
  • [N/A] I added or updated documentation (if applicable).
  • [N/A] I tried running the project locally and verified that there are no visible
    errors.

@fcoveram fcoveram requested a review from a team as a code owner March 14, 2023 10:50
@openverse-bot openverse-bot added this to Needs review in Openverse PRs Mar 14, 2023
@openverse-bot openverse-bot added the 🚦 status: awaiting triage Has not been triaged & therefore, not ready for work label Mar 14, 2023
@github-actions
Copy link

github-actions bot commented Mar 14, 2023

Full-stack documentation: Ready

https://WordPress.github.io/openverse/_preview/912

Please note that GitHub pages takes a little time to deploy newly pushed code, if the links above don't work or you see old versions, wait 5 minutes and try again.

You can check the GitHub pages deployment action list to see the current status of the deployments.

@fcoveram fcoveram added 🟧 priority: high Stalls work on the project or its dependents 📄 aspect: text Concerns the textual material in the repository labels Mar 14, 2023
@fcoveram fcoveram requested review from zackkrida and removed request for stacimc March 15, 2023 09:18
@fcoveram fcoveram mentioned this pull request Mar 15, 2023
7 tasks
@fcoveram fcoveram self-assigned this Mar 16, 2023
Copy link
Member

@zackkrida zackkrida left a comment

Choose a reason for hiding this comment

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

LGTM. I left some non-blocking suggestions to remove empty sections.

@openverse-bot
Copy link
Collaborator

Based on the high urgency of this PR, the following reviewers are being gently reminded to review this PR:

@dhruvkb
This reminder is being automatically generated due to the urgency configuration.

Excluding weekend1 days, this PR was updated 2 day(s) ago. PRs labelled with high urgency are expected to be reviewed within 2 weekday(s)2.

@panchovm, if this PR is not ready for a review, please draft it to prevent reviewers from getting further unnecessary pings.

Footnotes

  1. Specifically, Saturday and Sunday.

  2. For the purpose of these reminders we treat Monday - Friday as weekdays. Please note that the that generates these reminders runs at midnight UTC on Monday - Friday. This means that depending on your timezone, you may be pinged outside of the expected range.

@fcoveram
Copy link
Contributor Author

A question to @WordPress/openverse-frontend. Does this work need technical planning before it can begin?

@obulat
Copy link
Contributor

obulat commented Mar 21, 2023

I love the simplification that this project brings to the UI!

In terms of the implementation plan, @panchovm, having the size/variant (or a note saying that the button is non-standard) for each VButton on the site would be amazing. I am not sure if you would be comfortable doing that as you are not as familiar with the underlying code.

Some edge cases I saw when reviewing the code were:

  • Text buttons (without border, and an underline on hover) are not present in Figma anymore, and (according to @panchovm 's reply in Figma) are replaced with transparent gray buttons. One of them is the "Report this image/audio" button on the single result page. The button currently has no horizontal padding to make the text look aligned with the page layout. So, do we keep the no horizontal padding for this button, or do we add the standard padding there?

  • Note on sizes for any buttons that are not going to be standardized: the search button, play-pause button, the load more button, etc.

@obulat obulat added ✨ goal: improvement Improvement to an existing user-facing feature 🧱 stack: frontend Related to the Nuxt frontend and removed 🚦 status: awaiting triage Has not been triaged & therefore, not ready for work labels Mar 21, 2023
@fcoveram
Copy link
Contributor Author

I am not sure if you would be comfortable doing that as you are not as familiar with the underlying code.

I feel uncomfortable tackling this task because of the technical knowledge I lack.


To your edge case points @obulat

The button ["Report this image/audio"] currently has no horizontal padding to make the text look aligned with the page layout. So, do we keep the no horizontal padding for this button, or do we add the standard padding there?

We would need to remove the right padding of the section where the button is placed to make it look align with the rest of the content. Similarly as we did with the "Filters" button where only hover and active states show the button's hidden padding. Here is a screenshot of the header's spacing effect.

CleanShot 2023-03-21 at 15 23 09@2x

Note on sizes for any buttons that are not going to be standardized: the search button, play-pause button, the load more button, etc.

Two question come to me based on this point

  1. Is this a list we would need to create in the implementation plan to specify which components are and are not involved in this change?
  2. Do these VButton-based components (the search button, play-pause button, the load more button, etc) duplicate or inherit the VButton specs? Asking this to prevent modifying these VButton-based elements.

@zackkrida zackkrida removed the request for review from dhruvkb March 21, 2023 14:53
@zackkrida zackkrida requested a review from obulat March 21, 2023 14:53
@zackkrida
Copy link
Member

@obulat since you have already reviewed this and made multiple contributions, would you be comfortable acting as a reviewer instead of @dhruvkb? The conversation you and @panchovm are currently engaging in seems like the last remaining piece to finalize this proposal.

@obulat
Copy link
Contributor

obulat commented Mar 21, 2023

@obulat since you have already reviewed this and made multiple contributions, would you be comfortable acting as a reviewer instead of @dhruvkb? The conversation you and @panchovm are currently engaging in seems like the last remaining piece to finalize this proposal.

I was going to suggest that as a matter of fact :)

@sarayourfriend
Copy link
Contributor

I would love a special focus on the "Infrastructure" and "Success" sections as the former needs to assess the impact of this work, to then set the success criteria for the latter section.

To answer this directly: I don't think this has any infrastructural implications.

For "success" I can't think of anything that is actually useful that we could measure for this. I'd suggest defining the success of this project based on the completed implementation of it, but it's a bit tautological. If you wanted to avoid that, then defining the way in which the search experience is meant to be improved by this would help in scoping a "success". Is this only an aesthetic improvement, or should we also see changes in the usability of the site? If it's the latter, then would we be able to meaningfully measure them?

@obulat
Copy link
Contributor

obulat commented Mar 22, 2023

@panchovm, I've done an audit of the buttons, and have a couple of questions for you in the comments in this spreadsheet

Could you take a look at the spreadsheet and confirm that everything is correct?

The main question is whether we should add a filled-transparent variant for the pages menu button in the header (it should be transparent because it shows up on both white and yellow pages).

A couple of buttons can use the regular sizes for height, and override the width to become full-width.

@fcoveram
Copy link
Contributor Author

I don't have edit access to the spreadsheet. Maybe that's why I can not see the comments. I sent you a request.

Could you take a look at the spreadsheet and confirm that everything is correct?

I need to dive into the current pages and mockups to make sure everything is fine. Do you think it makes more sense to move this info to the Figma file? In that way, design specs and components would live in one source of truth.

The main question is whether we should add a filled-transparent variant for the pages menu button in the header

The page menu component is fine as it is. And this makes me think of adding an accurate list of the components excluded from this change.


I appreciate you could answer the question below I asked here.

Do these VButton-based components (the search button, play-pause button, the load more button, etc) duplicate or inherit the VButton specs? Asking this to prevent modifying these VButton-based elements.

My point here is that your audit document names some elements as primary, and by consequence a second and third might exist. But the specs I talked with @dhruvkb time ago changes this nomenclature to a per-style utility. That's why the components specs page in Figma shows this.

CleanShot 2023-03-22 at 09 48 54@2x

Note: In the gray box, the first text is the Storybook name whereas the texts below the divider are the new button specs

My concern with this logic is that primary is an arrange of classes with a 48px height, but that value is also in Filters and Switcher components. And that might confuse when other elements share similar characteristics but are placed and used differently as primary concept conveys.

@obulat
Copy link
Contributor

obulat commented Mar 22, 2023

I've updated the spreadsheet link, and replaced primary with pink in the button names there.

Sorry I missed your questions, @panchovm

Two question come to me based on this point

Is this a list we would need to create in the implementation plan to specify which components are and are not involved in this change?

I think we can review each button case by case in the implementation PR, if necessary.

Do these VButton-based components (the search button, play-pause button, the load more button, etc) duplicate or inherit the VButton specs? Asking this to prevent modifying these VButton-based elements.

These components inherit the VButton specs, but can override some properties. For example, we can use 'large' size and 'filled-gray' variant for the Load more button, but add a tailwind class of 'w-full' to it so that the width becomes 100% instead of the 'auto' that a normal VButton has:
VButton variant="filled-gray" size="large" class="w-full">Load more</VButton>.
For the search button, we currently use the plain style with no background and no border, and use the tailwind classes in the "VSearchButton" or "VSearchBar" components to set the colors for resting, hover and focus states.

I opened a draft PR #989 with all the styles from the Figma file to experiment with them. You can see the Storybook with variants and sizes here. It allows you to see several buttons of different styles on one page, and adjust the sizes using the selects below the canvas:
Screenshot 2023-03-22 at 6 14 24 PM

After doing this I realized that we don't have the "pressed" (and pressed-hovered) states in the Figma file.

@obulat
Copy link
Contributor

obulat commented Mar 22, 2023

Note: In the gray box, the first text is the Storybook name whereas the texts below the divider are the new button specs

My concern with this logic is that primary is an arrange of classes with a 48px height, but that value is also in Filters and Switcher components. And that might confuse when other elements share similar characteristics but are placed and used differently as primary concept conveys.

I renamed the items to "filled-pink" and "filled-dark".
Do you think there's still value in having the "Primary" and "Secondary" buttons? I think having the matrix of variants, sizes and 'icon-start'/'icon-end' is easier to manage because you can clearly see what the button is. And with Primary, you have to remember what size and color it is. And I don't think it's used in more that 1-2 places, or is it?

Copy link
Contributor

@obulat obulat left a comment

Choose a reason for hiding this comment

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

Really excited to get started!

Openverse PRs automation moved this from Needs review to Reviewer approved Mar 23, 2023
@fcoveram fcoveram merged commit f23ecb7 into main Mar 23, 2023
Openverse PRs automation moved this from Reviewer approved to Merged! Mar 23, 2023
@fcoveram fcoveram deleted the project-proposal-core-user-interface-improvement branch March 23, 2023 16:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📄 aspect: text Concerns the textual material in the repository ✨ goal: improvement Improvement to an existing user-facing feature 🟧 priority: high Stalls work on the project or its dependents 🧱 stack: frontend Related to the Nuxt frontend
Projects
Status: Accepted
Openverse PRs
  
Merged!
Development

Successfully merging this pull request may close these issues.

None yet

5 participants