Update close buttons #3225
Merged
Update close buttons #3225
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
openverse-bot
added
🟨 priority: medium
|
Size Change: +104 B (0%) Total Size: 939 kB
ℹ️ View Unchanged
|
obulat
changed the title
obulat
changed the title
fcoveram
requested changes
Oct 19, 2023
There was a problem hiding this comment.
The update looks great, I noticed some room for improvement to reach more consistency with other recent upgrades.
The license explanation popup
The button should be small / transparent / gray, and the icon change to close
The report popup
The button should be small / transparent / gray, and the icon change to close
The pages modal
The change is correct
The content settings modal
The size (large) is correct, but it needs to be transparent / gray, and the icon change to close
Notification bar
The change is correct
Modal content
The change is correct
obulat
force-pushed
the
fix/license_explanation_button_focus_ring
branch
from
96fa70f to
4b802f1
Compare
|
Buttons updated The license explanation popup
The report popup
The content settings modal
The snapshots for the content settings modal and report popover were updated. |
obulat
force-pushed
the
fix/license_explanation_button_focus_ring
branch
from
3cabf1d to
761ca66
Compare
obulat
force-pushed
the
fix/license_explanation_button_focus_ring
branch
from
761ca66 to
95d8bfd
Compare
|
Based on the medium urgency of this PR, the following reviewers are being gently reminded to review this PR: @AetherUnbound Excluding weekend1 days, this PR was ready for review 5 day(s) ago. PRs labelled with medium urgency are expected to be reviewed within 4 weekday(s)2. @obulat, if this PR is not ready for a review, please draft it to prevent reviewers from getting further unnecessary pings. Footnotes
|
dhruvkb
approved these changes
Oct 27, 2023
There was a problem hiding this comment.
Is the VCloseButton component being deprecated/removed?
Nothing against this PR, the changes LGTM but I have some objection to the design itself that I want to bring to @fcoveram's notice. The cross button is now much bigger than all other iconography in the app.
If need be, it can be tackled separately.
Yes, it's being removed because it's using the old button underneath. There's one CloseButton left in the Global audio player, but that should be updated separately because the global audio player styles have changed.
@fcoveram, I'm going to merge this PR, but if you agree with @dhruvkb, we can update the size in a different PR. |
|
I agree the icon variant looks bigger, but the idea is to be consistent with modal and dialogs hierarchy. Modals (metasearch, settings drawer, and media details' designs) and popovers (report, and license explanation) use the big version, whereas messages use the small one. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes
Fixes #467 by @obulat
Description
This PR replaces
VCloseButtonwithVIconButtonfor the close button in several components. This uses the updated button styles and removes the old button styles from the app.The license explanation popup
On desktop widths
The report popup
Single result pages, both desktop and mobile widths
The pages modal
Content pages, i.e.
/about, on mobile widthsThe content settings modal
Search page on mobile widths
Notification bar
Dark mode on homepage, light mode on the search page
Modal content
External sources form, images search page
This is a part of the effort of replacing
VOldIconButtonthat is used byVCloseButton. Now, the only component that usesVCloseButtonis the global audio player, which will be updated in a separate PR since the design was changed recently.Testing Instructions
Run the app and open the search page (localhost:8443/search/?q=cat) on a desktop width.
For the license explanation button, open the filters sidebar and click on a license explanation button
(?). Check that the close button's focus ring looks correct.Check other buttons listed in the description of the PR.
Checklist
Update index.md).main) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin