Skip to content
This repository has been archived by the owner on Mar 13, 2024. It is now read-only.

Select Team list container should scale in width based on browser window width #14197 #5833

Merged
merged 5 commits into from
Jul 22, 2020
Merged

Select Team list container should scale in width based on browser window width #14197 #5833

merged 5 commits into from
Jul 22, 2020

Conversation

sridhar02
Copy link
Contributor

Summary

changes done for the issues mentioned in the ticket

Ticket Link

Fixes mattermost/mattermost#14197
JIRA https://mattermost.atlassian.net/browse/MM-23749

Related Pull Requests

  • Has server changes (please link here)
  • Has redux changes (please link here)
  • Has mobile changes (please link here)

Screenshots

  • overflow-y addition on the mobile view
    image

  • Before the change when the max-width was 400px

image

  • after the max-width is set to 800px

image

@hanzei hanzei requested a review from esethna July 3, 2020 12:28
@hanzei hanzei added 1: PM Review Requires review by a product manager 2: Dev Review Requires review by a core commiter 3: QA Review Requires review by a QA tester Setup Cloud Test Server Setup a test server using Mattermost Cloud labels Jul 3, 2020
@esethna esethna added the 1: UX Review Requires review by a UX Designer label Jul 3, 2020
@esethna esethna added this to the v5.26 milestone Jul 3, 2020
@esethna
Copy link
Contributor

esethna commented Jul 3, 2020

Thanks @sridhar02, looks good from my end! Adding @andrewbrown00 for a UX review

@esethna esethna removed the request for review from andrewbrown00 July 9, 2020 21:06
@esethna esethna removed 1: PM Review Requires review by a product manager 1: UX Review Requires review by a UX Designer labels Jul 9, 2020
@jgilliam17
Copy link
Contributor

/update-branch

@jgilliam17
Copy link
Contributor

I noticed login screen email/password fields width has changed on the test server.
Is this intentional?
Screen Shot 2020-07-10 at 12 16 30 PM

@sridhar02
Copy link
Contributor Author

sridhar02 commented Jul 14, 2020 •
edited
Loading

@jgilliam17,actullay it was mentioned in the task to change the width of the list container to 800px, I have changed it and it is effecting the sign-in also as they are using the same styles for both the teams and the sign-in pages.
image

@andrewbrown00
Copy link

@sridhar02 thanks for the contribution here 👍

Changes to the Teams page looks good ✅

image

Item to resolve

  • Revert the Login page back the previous width, it's too wide
Current Expected (previous)
image image

@jgilliam17
Copy link
Contributor

Thank you @sridhar02
Team list page looks good.

@andrewbrown00 found two more instance where field width was increased. Should this also be reverted? Team URL screen actually looks better with the wider field, where most of the url is visible.

Create a Team

Screen Shot 2020-07-14 at 10 31 29 AM

Team URL screen

Screen Shot 2020-07-14 at 10 32 12 AM

@andrewbrown00
Copy link

@andrewbrown00 found two more instance where field width was increased. Should this also be reverted? Team URL screen actually looks better with the wider field, where most of the url is visible.

Thanks @jgilliam17, yes, all non-team page instances should be reverted 🙂

@sridhar02
Copy link
Contributor Author

@jgilliam17 @andrewbrown00 I have reverted the changes and made changes that will be applicable to the team container u can check it.

@andrewbrown00
Copy link

andrewbrown00 commented Jul 15, 2020 •
edited
Loading

@jgilliam17 @andrewbrown00 I have reverted the changes and made changes that will be applicable to the team container u can check it.

Looks good @sridhar02 from my end. Thanks for quick updates 🙂 , I'll let @jgilliam17 do her review too.

jgilliam17
jgilliam17 approved these changes Jul 15, 2020
View reviewed changes
Copy link
Contributor

@jgilliam17 jgilliam17 left a comment

Choose a reason for hiding this comment

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

Thank you @sridhar02 for a quick fix reverting login and team creation screen fields to previous width.
Tested, looks good to merge.

  • Verified Team list scales in width based on browser window width.

@jgilliam17 jgilliam17 added QA Review Done and removed 3: QA Review Requires review by a QA tester labels Jul 15, 2020
calebroseland
calebroseland suggested changes Jul 17, 2020
View reviewed changes
Copy link
Member

@calebroseland calebroseland left a comment

Choose a reason for hiding this comment

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

LGTM! Just one comment related to the overflow behavior.

components/select_team/select_team.jsx
@@ -336,7 +336,7 @@ export default class SelectTeam extends React.PureComponent {
{headerButton}
<div className='col-sm-12'>
<div
className={'signup-team__container'}
className={'select-team__container signup-team__container'}
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for isolating, I've been thinking we need a unique class to distinguish some of these screens.

sass/responsive/_mobile.scss Outdated

.signup-team-all .infinite-scroll {
overflow-y: auto;
}
Copy link
Member

Choose a reason for hiding this comment

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

This selector doesn't seem to hit the element I presume that it targets.

<div class="infinite-scroll signup-team-all">

?

.signup-team-all.infinite-scroll {
    overflow-y: auto;
}

digging into this further, though.. I don't know that this is even needed, as the following already kicks in on hover inside (e.g. you start to drag on mobile)

.infinite-scroll:hover {
    overflow-y: scroll;
    overflow-y: overlay;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I have not checked in that way, I think it working on the hover, so I deleted this class now

.signup-team-all.infinite-scroll {
    overflow-y: auto;
}

@amyblais amyblais removed this from the v5.26 milestone Jul 17, 2020
@sridhar02 sridhar02 mentioned this pull request Jul 20, 2020
Merged
calebroseland
calebroseland approved these changes Jul 20, 2020
View reviewed changes
Copy link
Member

@calebroseland calebroseland left a comment

Choose a reason for hiding this comment

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

Looks good ✓

sudheerDev
sudheerDev approved these changes Jul 22, 2020
View reviewed changes
Copy link
Contributor

@sudheerDev sudheerDev left a comment

Choose a reason for hiding this comment

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

LGTM

@sudheerDev
Copy link
Contributor

Thank you @sridhar02

@sudheerDev sudheerDev added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core commiter Setup Cloud Test Server Setup a test server using Mattermost Cloud labels Jul 22, 2020
@mm-cloud-bot
Copy link

Test server destroyed

@sudheerDev sudheerDev merged commit 7141560 into mattermost:master Jul 22, 2020
@amyblais amyblais added this to the v5.28 milestone Aug 13, 2020
@amyblais amyblais added Changelog/Done Required changelog entry has been written Docs/Not Needed Does not require documentation labels Sep 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@sudheerDev sudheerDev sudheerDev approved these changes

@calebroseland calebroseland calebroseland approved these changes

@esethna esethna esethna approved these changes

@jgilliam17 jgilliam17 jgilliam17 approved these changes

Assignees
No one assigned
Labels
4: Reviews Complete All reviewers have approved the pull request Changelog/Done Required changelog entry has been written Docs/Not Needed Does not require documentation QA Review Done
Projects
None yet
Milestone
v5.28.0
9 participants
@sridhar02 @esethna @jgilliam17 @andrewbrown00 @sudheerDev @mm-cloud-bot @calebroseland @hanzei @amyblais