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

Make the descriptions on the intro pages scrollable #964

Closed
wants to merge 3 commits into from

Conversation

EasyVector
Copy link

Dear developer:

Hello! I am the creator of issue #963, and I have fixed the blocked descriptions to make it more responsive. I would appreciate it if you could kindly revise my code and leave me some advice. Thank you so much for your precious time!

Here are the screenshots after my changes:

copy copy copy

Thanks again! Blessings on your day! :)

@franklinbill
Copy link

I met this mistake again. Great! I agree with your fix 👍 @yuhuitech

Copy link
Member

@cortinico cortinico left a comment

Choose a reason for hiding this comment

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

Thanks for your PR 🙏

The layout-land file should also be adapted.

<TextView
android:id="@+id/description"
style="@style/AppIntroDefaultText"
<ScrollView
Copy link
Member

Choose a reason for hiding this comment

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

The scrollview is not needed. It should be enought with:

android:scrollbars = "vertical"

Plus

textView.setMovementMethod(new ScrollingMovementMethod());

Copy link
Author

Choose a reason for hiding this comment

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

OK, I will work on this right now. :)
Thank you so much!

Copy link
Author

@EasyVector EasyVector Aug 30, 2021

Choose a reason for hiding this comment

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

Hi! Those two methods seem to have the same effect as far as I am concerned. And I also guess it would be easier to maintain the project if we let the XML do all the rendering work. Editing Java files would also work, but maybe it is harder for other developers to associate it with the XML file. This is my opinion. I would appreciate it if you could give me some advice on this. Thank you so much! :)

Copy link
Author

@EasyVector EasyVector Aug 30, 2021

Choose a reason for hiding this comment

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

If it is OK with you I would like to do the adaptation work of the landscape mode. :)

Copy link
Member

Choose a reason for hiding this comment

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

Let's go with the XML only approach 👍

Copy link
Member

Choose a reason for hiding this comment

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

Please note this:

The scrollview is not needed.

You can remove the ScrollView entirely 👍

Copy link
Author

Choose a reason for hiding this comment

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

Hi! Sorry for the wait. I guess I have solved the problem when the landscape mode is used. Here are some screenshots after modifications when using the largest display size:

copy

copy

copy

and the smallest display size:

copy

copy

I am looking forward to your review. Thanks again for your kindness and your precious time!! :)

Copy link
Author

Choose a reason for hiding this comment

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

You can remove the ScrollView entirely

If I remove the ScrollView, should I use the java code you recommended earlier?

Copy link
Member

Choose a reason for hiding this comment

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

Please do your own researches/trial and let me know what's the outcome.
The solution with the least amount of changes is generally preferred.

Like if we can solve it by just adding an XML attribute, that would be the preferred approach.

Copy link
Author

Choose a reason for hiding this comment

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

OK, I could comprehend the meaning you are trying to deliver to me now. Therefore I would like to make a comparison of those two methods and I would like to inform you of this once it is ready. And I will also minimize the changes I could apply to those files. :)
Thanks again!

@EasyVector
Copy link
Author

Hello! Sorry for the wait. After my research on this project and this problem, I would like to recommend you modify XML files only. Here is the reason.

I did some investigations on this project and I found that maybe it is the "AppIntroBaseFragment.kt" file that controls the creation of the views on the Intro pages, which is this function onCreateView, more specifically. I applied some changes to it, just like this:

    override fun onCreateView(
        inflater: LayoutInflater,
        container: ViewGroup?,
        savedInstanceState: Bundle?
    ): View? {
        val view = inflater.inflate(layoutId, container, false)
        val titleText = view.findViewById<TextView>(R.id.title)
        val descriptionText = view.findViewById<TextView>(R.id.description)
        val slideImage = view.findViewById<ImageView>(R.id.image)
        val mainLayout = view.findViewById<ConstraintLayout>(R.id.main)

        // Make the description texts scrollable
        descriptionText.setMovementMethod(ScrollingMovementMethod())

        titleText.text = title
        descriptionText.text = description
        if (titleColor != 0) {
            titleText.setTextColor(titleColor)
        }
        if (descColor != 0) {
            descriptionText.setTextColor(descColor)
        }
        titleTypeface?.applyTo(titleText)
        descTypeface?.applyTo(descriptionText)

        slideImage.setImageResource(drawable)
        if (bgDrawable != 0) {
            mainLayout?.setBackgroundResource(bgDrawable)
        } else {
            mainLayout?.setBackgroundColor(defaultBackgroundColor)
        }

        return view
    }

In the meantime, I also revoked all the changes I applied to those two XML files. Then I started my test.

Here are some normal screenshots after my test, and in such circumstances, those text views could be seen fully:

copy copy

And it also could handle the situation where an extremly longer sentence is in the text view.

copy

However, this screenshot is not that charming when it comes to the land mode, mainly for the title. As you could see, a part of the title is blocked by its border. But the description is scrollable.

copy

According to my comparison, I found some layout issues in the land mode, which are caused by the XML file. So I guess the land mode XML file needs a repair. From the screenshots I have shown you before, I reckon that I have tackled this problem without changing any kt or java code, just some modifications on the XML files. That is, after my modifications, the long title on the land mode is not blocked.

So my final conclusion is that perhaps we don't need to change any kt or java files but only XML files to reach this goal.

That is my opinion. I am very humble and sincere so I would be so glad if you could leave me some advice on this. And I would like to make contributions to this project with all of my best efforts. :)

Thanks again!

@cortinico
Copy link
Member

Thanks for your clarification and for taking the time to look into this,

So my final conclusion is that perhaps we don't need to change any kt or java files but only XML files to reach this goal.

Are you suggesting we go with the ScrollView approach right?

@EasyVector
Copy link
Author

Yes, we should go with the ScrollView approach. :)

@cortinico
Copy link
Member

Yes, we should go with the ScrollView approach. :)

I would like to take a view at my implementation here: 91909ec

It doesn't use the ScrollView yet achieves the same solution.
I believe the only problem left is the Title in Land mode, but I think we should tune the text autosizing for that specific scenario.

@EasyVector
Copy link
Author

EasyVector commented Sep 6, 2021

Yes, I also believe tuning is necessary and I would like to try your code.

And I also would like to add that since those introduction pages are static and they don't need changes during running time, I believe it would be better to declare UI layouts in XML files considering performance, speed as such.

@EasyVector EasyVector changed the title Made the descriptions on the intro pages scrollable Make the descriptions on the intro pages scrollable Sep 6, 2021
@cortinico
Copy link
Member

Yes, I also believe tuning is necessary and I would like to try your code.

What's the status of this PR?

@EasyVector
Copy link
Author

Yes, I also believe tuning is necessary and I would like to try your code.

What's the status of this PR?

Hello! I am quite busy recently, and I have tested your code already, and I think they are good to go. Making those text scrollable is a wonderful choice.

@cortinico
Copy link
Member

Thanks for the feedback. I believe we might want to include this change into #978

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.

None yet

3 participants