Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

Closes #2316: Adds BrowserMenuCaption #2317

Merged
merged 1 commit into from
Mar 11, 2019
Merged

Conversation

sblatz
Copy link
Contributor

@sblatz sblatz commented Mar 11, 2019

Pull Request checklist

  • Quality: This PR builds and passes detekt/ktlint checks (A pre-push hook is recommended)
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Changelog: This PR includes a changelog entry or does not need one
  • Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features

@sblatz sblatz requested a review from a team as a code owner March 11, 2019 17:47
@Amejia481 Amejia481 self-assigned this Mar 11, 2019
val item = BrowserMenuCaption("Powered by Mozilla")
assertEquals(R.layout.mozac_browser_menu_item_caption, item.getLayoutResource())
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a test to check the text is actually set? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I didn't think there was a way to test this, but I looked at BrowserMenuImageTextTest for some inspiration. I've added a few more, but please let me know if that's not sufficient!

@codecov
Copy link

codecov bot commented Mar 11, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@613b13c). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #2317   +/-   ##
=========================================
  Coverage          ?   84.52%           
  Complexity        ?     2631           
=========================================
  Files             ?      315           
  Lines             ?    10658           
  Branches          ?     1555           
=========================================
  Hits              ?     9009           
  Misses            ?      997           
  Partials          ?      652
Impacted Files Coverage Δ Complexity Δ
...ponents/browser/menu/item/SimpleBrowserMenuItem.kt 77.77% <ø> (ø) 3 <0> (?)
...components/browser/menu/item/BrowserMenuCaption.kt 100% <100%> (ø) 4 <4> (?)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 613b13c...fbef553. Read the comment docs.

@Amejia481
Copy link
Contributor

@sblatz what is the difference between BrowserMenuCaption and SimpleBrowserMenuItem?
it's only the text size and the no click listener attach to it, can we add it SimpleBrowserMenuItem`?

@sblatz
Copy link
Contributor Author

sblatz commented Mar 11, 2019

@Amejia481 Yes, those are the only differences. I'm happy to add a textSize param to SimpleBrowserMenuItem, but I also didn't want tapping on it to dismiss the menu, which is why I ended up creating a separate item. Is there another way I can get around this?

@Amejia481
Copy link
Contributor

Amejia481 commented Mar 11, 2019

@sblatz we could make the listener optional and only set the click listener when is not null

class SimpleBrowserMenuItem(
        private val label: String,
        @ColorRes
        private val textColorResource: Int = NO_ID,
        private val listener: (() -> Unit)? = null
) : BrowserMenuItem {
    override var visible: () -> Boolean = { true }

    override fun getLayoutResource() = R.layout.mozac_browser_menu_item_simple

    override fun bind(menu: BrowserMenu, view: View) {
        (view as TextView).text = label
        if (textColorResource != NO_ID) {
            view.setTextColor(ContextCompat.getColor(view.context, textColorResource))
        }

        if (listener != null) {
            view.setOnClickListener {
                listener.invoke()
                menu.dismiss()
            }
        } else {
            // This will remove the ripple effect.
            view.background = null
        }
    }
}

@sblatz
Copy link
Contributor Author

sblatz commented Mar 11, 2019

@Amejia481 seems like a great solution. I'll make that change!

@sblatz sblatz force-pushed the fix-2316 branch 3 times, most recently from b5821e0 to a1a1e35 Compare March 11, 2019 20:33
@Amejia481 Amejia481 force-pushed the fix-2316 branch 2 times, most recently from beb0bfd to 2726c54 Compare March 11, 2019 21:19
@sblatz sblatz merged commit 1b1600a into mozilla-mobile:master Mar 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants