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

fix: toast to snackbar for login work #788

Merged
merged 10 commits into from Dec 30, 2018
Merged

fix: toast to snackbar for login work #788

merged 10 commits into from Dec 30, 2018

Conversation

GOVINDDIXIT
Copy link
Member

@GOVINDDIXIT GOVINDDIXIT commented Dec 28, 2018

Part for #785
fix #797
merge after #790

Screenshots for Login fragment UI fix
screenshot_2018-12-30-13-19-27-496_com eventyay attendee
screenshot_2018-12-30-13-19-38-194_com eventyay attendee

GIF for the Snackbar change:
ezgif com-video-to-gif 2

})

loginViewModel.loggedIn.observe(this, Observer {
Toast.makeText(context, getString(R.string.welcome_back), Toast.LENGTH_LONG).show()
Snackbar.make(getActivity()!!.findViewById(android.R.id.content),
Copy link
Member

Choose a reason for hiding this comment

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

use safe calls instead of !!

Copy link
Member Author

Choose a reason for hiding this comment

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

but then it is showing type mismatch as snackbar requires view of type View and we are giving View? Are non-null asserted calls allowed?

Copy link
Member

Choose a reason for hiding this comment

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

First of all, don't use findViewById. Secondly, that'll fix it

Copy link
Member

Choose a reason for hiding this comment

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

android.R.id.content This isn't to be used. Use coordinator layout of the current fragment/activity

Copy link
Member Author

Choose a reason for hiding this comment

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

Should I open an issue to change layout to coordinator layout or should do in this PR only. Also if we use coordinator layout of the current fragment as a view then snackbar will not be coming from bottom of the screen. Pls correct me

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, OK. Use CoordinatorLayout of the root element

Copy link
Member Author

Choose a reason for hiding this comment

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

but how to get CoordinatorLayout of the root element. when I try to use CoordinatorLayout of root element i.e rootLayout(activity_main.xml) It is becoming null
I'm facing
java.lang.IllegalStateException: rootView.findViewById(R.id.rootLayout) must not be null at org.fossasia.openevent.general.order.OrdersUnderUserFragment.onCreateView(OrdersUnderUserFragment.kt:48)
Need help

Copy link
Member

Choose a reason for hiding this comment

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

Call findViewById on activity

@fossasia fossasia deleted a comment Dec 28, 2018
@fossasia fossasia deleted a comment Dec 28, 2018
@fossasia fossasia deleted a comment Dec 28, 2018
@fossasia fossasia deleted a comment Dec 28, 2018
@fossasia fossasia deleted a comment Dec 28, 2018
@fossasia fossasia deleted a comment Dec 28, 2018
@fossasia fossasia deleted a comment Dec 28, 2018
@fossasia fossasia deleted a comment Dec 28, 2018
@GOVINDDIXIT GOVINDDIXIT changed the title fix: toast to snackbar for login work [wip] fix: toast to snackbar for login work Dec 28, 2018
@fossasia fossasia deleted a comment Dec 28, 2018
@iamareebjamal
Copy link
Member

Your PR has undone several changes

@GOVINDDIXIT
Copy link
Member Author

Yes. Sorry for that I will correct them using different branch

@GOVINDDIXIT GOVINDDIXIT changed the title [wip] fix: toast to snackbar for login work fix: toast to snackbar for login work Dec 28, 2018
iamareebjamal pushed a commit that referenced this pull request Dec 28, 2018
* chore: change to coordinator layout requires in #788

* Update fragment_login.xml

* Update fragment_login.xml
@GOVINDDIXIT
Copy link
Member Author

@iamareebjamal the UI of login fragment is not fine with the requested changes in #790 The preview is not building correctly due to current changes in gradle which is making difficult to fix this Should I go with old way with scrollview wrapping coordinator layout which had fix this. Pls guide
screenshot_2018-12-30-12-37-04-235_com eventyay attendee
screenshot_2018-12-30-12-25-53-760_com eventyay attendee

@iamareebjamal
Copy link
Member

OK

@@ -56,7 +58,7 @@ class LoginFragment : Fragment() {
savedInstanceState: Bundle?
): View? {
rootView = inflater.inflate(R.layout.fragment_login, container, false)

CoordinatorLayout = rootView.findViewById(R.id.loginCoordinatorLayout)
Copy link
Member

Choose a reason for hiding this comment

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

Don't use findViewById

Copy link
Member

Choose a reason for hiding this comment

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

I have no idea why is this capital. Is this a class?

CoordinatorLayout, "You need to log in first!", Snackbar.LENGTH_SHORT).show()
Handler().postDelayed({
redirectToLogin()
}, 1000)
Copy link
Member

Choose a reason for hiding this comment

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

Strange indentation

Copy link
Member Author

Choose a reason for hiding this comment

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

will correct don't know why autoformat is going for this

@GOVINDDIXIT
Copy link
Member Author

GOVINDDIXIT commented Dec 30, 2018

@iamareebjamal I think the current way to show snackbar as shown in GIF is more apt as showing snackbar from bottom of screen will hinder the UI nor it is properly hiding it. It is hiding navigation buttons upto 3/4. If you agree with current way I will move toward for requested changes. Pls guide

@@ -36,6 +38,7 @@ class LoginFragment : Fragment() {

private val loginViewModel by viewModel<LoginViewModel>()
private lateinit var rootView: View
private lateinit var coordinatorLayout: CoordinatorLayout
Copy link
Member

Choose a reason for hiding this comment

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

Why do you need to initialize it?

You have access to rootView. Use it everywhere

Copy link
Member Author

Choose a reason for hiding this comment

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

Step 'ktlint' found problem in 'app\src\main\java\org\fossasia\openevent\general\auth\LoginFragment.kt':
Error on line: 90, column: 1
Exceeded max line length (120)

@iamareebjamal can we increase this 120 length limit😅

Copy link
Member Author

Choose a reason for hiding this comment

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

on directly using rootview.requiredCoordinatorLayout. I am getting above error

Copy link
Member

Choose a reason for hiding this comment

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

Then break that line

@iamareebjamal iamareebjamal merged commit f556432 into fossasia:development Dec 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fix login fragment UI
3 participants