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.

Sign up for GitHub

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

[16845] Wrong buttons background color on community home screen's page nav #17003

Merged
merged 12 commits into from
Oct 3, 2023
Merged

[16845] Wrong buttons background color on community home screen's page nav #17003

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
93dd0e5
Fix blur of button when overlay is shown
ibrkhalil Oct 1, 2023
0531adb
lint
ibrkhalil Oct 1, 2023
e31e5ab
JC's comments
ibrkhalil Oct 1, 2023
2a1d7fc
Lint
ibrkhalil Oct 1, 2023
44d9dac
Formatting
ibrkhalil Oct 3, 2023
4d80851
Merge branch 'develop' into 16845
ibrkhalil Oct 3, 2023
008c634
Merge branch 'develop' into 16845
ibrkhalil Oct 3, 2023
598ca48
Merge branch 'develop' into 16845
ibrkhalil Oct 3, 2023
75ae50b
Merge branch 'develop' into 16845
ibrkhalil Oct 3, 2023
52578c3
Merge branch 'develop' into 16845
ibrkhalil Oct 3, 2023
dee0dc2
Liint
ibrkhalil Oct 3, 2023
d5ac41c
Comment style
ibrkhalil Oct 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 14 additions & 8 deletions src/quo2/components/navigation/page_nav/view.cljs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
:blur :grey})

(defn- page-nav-base
[{:keys [margin-top background on-press accessibility-label icon-name]
[{:keys [margin-top background on-press accessibility-label icon-name overlay-shown?]
Copy link
Contributor

Choose a reason for hiding this comment

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

seems like a good solution, perhaps it needs a commit to explain why it is needed? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean in the commit message or a comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just in case it's a comment, I'll do it now

Copy link
Contributor

Choose a reason for hiding this comment

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

also a nit: perhaps behind-overlay? is a clearer name for this prop? 🤨

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep I meant in a comment

:or {background :white}}
& children]
(into [rn/view {:style (style/container margin-top)}
Expand All @@ -30,15 +30,17 @@
:icon-only? true
:size 32
:on-press on-press
:background (when (button-properties/backgrounds background) background)
:background (if overlay-shown?
ibrkhalil marked this conversation as resolved.
Show resolved Hide resolved
:blur
(when (button-properties/backgrounds background) background))
:accessibility-label accessibility-label}
icon-name])]
children))

(defn- right-section-spacing [] [rn/view {:style style/right-actions-spacing}])

(defn- add-right-buttons-xf
[max-actions background]
[max-actions background overlay-shown?]
(comp (filter map?)
(take max-actions)
(map (fn [{:keys [icon-name label] :as button-props}]
Expand All @@ -48,7 +50,9 @@
:icon-only? icon-name
:size 32
:accessible true
:background (when (button-properties/backgrounds background) background))
:background (if overlay-shown?
Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps put this logic into a simple helper function and leave a comment by that function explaining it's purpose there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We just check if the background is within a set of button types, Here's the definition of backgrounds.

Copy link
Contributor

Choose a reason for hiding this comment

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

I more so meant this because of the overlay part. It's somewhat of a design hack and so isolating to its own function and leaving the comment there hopefully makes the reason for this more apparent/easy to see

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's better to have a comment explaining it than having a function for it.

:blur
(when (button-properties/backgrounds background) background)))
(or label icon-name)]))
(interpose [right-section-spacing])))

Expand All @@ -64,7 +68,8 @@
emoji])

(defn- right-content
[{:keys [background content max-actions min-size? support-account-switcher? account-switcher]
[{:keys [background content max-actions min-size? support-account-switcher? account-switcher
overlay-shown?]
:or {support-account-switcher? true}}]
[rn/view (when min-size? {:style style/right-content-min-size})
(cond
Expand All @@ -73,7 +78,7 @@

(coll? content)
(into [rn/view {:style style/right-actions-container}]
(add-right-buttons-xf max-actions background)
(add-right-buttons-xf max-actions background overlay-shown?)
content)

:else
Expand Down Expand Up @@ -170,7 +175,7 @@
shown-name]]))

(defn- view-internal
[{:keys [type right-side background text-align account-switcher]
[{:keys [type right-side background text-align account-switcher overlay-shown?]
:or {type :no-title
text-align :center
right-side :none
Expand All @@ -179,7 +184,8 @@
(case type
:no-title
[page-nav-base props
[right-content {:background background :content right-side :max-actions 3}]]
[right-content
{:background background :content right-side :max-actions 3 :overlay-shown? overlay-shown?}]]

:title
(let [centered? (= text-align :center)]
Expand Down
19 changes: 10 additions & 9 deletions src/status_im2/common/scroll_page/view.cljs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@

(defn f-scroll-page-header
[scroll-height height name page-nav-right-side logo sticky-header top-nav title-colum navigate-back?
collapsed?]
collapsed? overlay-shown?]
(let [input-range [0 10]
output-range [-208 -45]
y (reanimated/use-shared-value scroll-height)
Expand Down Expand Up @@ -69,12 +69,13 @@
[rn/view {:style {:margin-top 0}}
top-nav]
[quo/page-nav
(cond-> {:margin-top 44
:type :no-title
:background (if (= 1 (reanimated/get-shared-value opacity-animation))
:blur
:photo)
:right-side page-nav-right-side}
(cond-> {:margin-top 44
:type :no-title
:background (if (= 1 (reanimated/get-shared-value opacity-animation))
:blur
:photo)
:right-side page-nav-right-side
:overlay-shown? overlay-shown?}
navigate-back? (assoc :icon-name :i/close
:on-press #(rf/dispatch [:navigate-back])))])
(when title-colum
Expand Down Expand Up @@ -106,12 +107,12 @@
(let [scroll-height (reagent/atom negative-scroll-position-0)]
(fn [{:keys [name theme cover-image logo page-nav-right-section-buttons on-scroll
collapsed?
height top-nav title-colum background-color navigate-back?]}
height top-nav title-colum background-color navigate-back? overlay-shown?]}
sticky-header
children]
[:<>
[:f> f-scroll-page-header @scroll-height height name page-nav-right-section-buttons
logo sticky-header top-nav title-colum navigate-back? collapsed?]
logo sticky-header top-nav title-colum navigate-back? collapsed? overlay-shown?]
[rn/scroll-view
{:content-container-style {:flex-grow 1}
:content-inset-adjustment-behavior :never
Expand Down
14 changes: 8 additions & 6 deletions src/status_im2/contexts/communities/overview/view.cljs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -312,9 +312,10 @@
;; user is on this page
initial-joined? joined]
(fn [{:keys [id name images] :as community} pending?]
(let [cover {:uri (get-in images [:banner :uri])}
logo {:uri (get-in images [:thumbnail :uri])}
collapsed? (and initial-joined? (:joined community))]
(let [cover {:uri (get-in images [:banner :uri])}
logo {:uri (get-in images [:thumbnail :uri])}
collapsed? (and initial-joined? (:joined community))
overlay-shown? (boolean (:sheets (rf/sub [:bottom-sheet])))]
[scroll-page/scroll-page
{:cover-image cover
:collapsed? collapsed?
Expand All @@ -324,7 +325,8 @@
:on-scroll #(reset! scroll-height %)
:navigate-back? true
:background-color (colors/theme-colors colors/white colors/neutral-95)
:height 148}
:height 148
:overlay-shown? overlay-shown?}
[sticky-category-header
{:enabled (> @scroll-height @first-channel-height)
:label (pick-first-category-by-height
Expand All @@ -336,8 +338,8 @@
pending?
{:on-category-layout (partial add-category-height categories-heights)
:collapsed? collapsed?
:on-first-channel-height-changed
;; Here we set the height of the component and we filter out the
:on-first-channel-height-changed ;; Here we set the height of the component and we filter out
ibrkhalil marked this conversation as resolved.
Show resolved Hide resolved
;; the
;; categories, as some might have been removed
(fn [height categories]
(swap! categories-heights select-keys categories)
Expand Down