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

Flexibus/#381/team page design #465

Merged
merged 38 commits into from
Feb 5, 2018
Merged

Flexibus/#381/team page design #465

merged 38 commits into from
Feb 5, 2018

Conversation

deeps96
Copy link

@deeps96 deeps96 commented Jan 29, 2018

Closes #381, also fixes #396.

Make sure to checkout flexibus/#381/team-page-design when reviewing since there exists a branch with a similar name.

deeps96 added 26 commits January 18, 2018 20:08
…ing -> waiting for ticket #338 to be successfully completed. #381
@ghost ghost assigned deeps96 Jan 29, 2018
@ghost ghost added the review label Jan 29, 2018
@deeps96 deeps96 requested review from a team and bakoe January 29, 2018 21:49
@JakobEdding
Copy link
Member

JakobEdding commented Jan 30, 2018

I'm changing the name of PR #379 to clear that up a bit at least because this has confused me before as well...

@lheilmann
Copy link
Contributor

@deeps96 Oh I am sorry! I misunderstood the scope of this issue/PR. I actually checked out the correct branch, but was focusing on the wrong pages. We use a different naming, e.g. teams index page, teams show page, teams edit page, ...

@bakoe
Copy link
Contributor

bakoe commented Feb 1, 2018

bildschirmfoto 2018-02-01 um 23 43 53

  • Should the kinds of sport (“Sportarten”) section really be displayed if the team has none assigned? (as it is the case for the team displayed in the screenshot)
  • Isn't there a colon : missing after “… folgende Sportarten”? (compared to the mockup from the user story Flexibus/#381/team page design #465)

Response 1

The colon is missing because of uniformity and cleaner design. @zhamanakov are you ok with that?
Is it a good idea to letting a team having no kinds of sport? I can fix this easily, but just a thought...

ANSWER: pls just hide the whole section, if there are no sports selected. DONE!


bildschirmfoto 2018-02-01 um 23 54 49

  • The kinds of sport (“Sportarten”) section could need some padding on the left and right sides

Response 1

What do you mean exactly?

Response by @bakoe

Sorry—I have posted the wrong screenshot for the problem I described. However, the issue is rather unimportant: When there is a very long (and thus multi-line) text in the kinds of sport (“Sportarten”) section, the text has very little space (CSS: padding) to the left and right ends of the section.


bildschirmfoto 2018-02-01 um 23 47 01

  • The margin between the Select all and Select none button seems a bit small on tablet and desktop layouts

Response 1

@zhamanakov Is this important? If yes, how much margin do you want?

ANSWER: min 20px margin between buttons is important DONE


bildschirmfoto 2018-02-01 um 23 47 20

  • The Select none button is neither spaced in the same way as in the tablet/desktop layout nor right-aligned in the phone layout

Response 1

Will fix DONE


bildschirmfoto 2018-02-01 um 23 54 49

  • The Select all and Select none buttons should not be rendered when there are no checkboxes rendered

Response 1

Will fix that. But just for interest: When is this the case?

Response by @bakoe

Thanks! This situation is the case whenever you look at the team page of a (public) team you are not a member of.

DONE

bildschirmfoto 2018-02-01 um 23 52 05

  • The font sizes of the different button groups are considerably different from each other – in the mockup from the user story Flexibus/#381/team page design #465 they all look pretty much the same

Response 1

Will fix DONE

@bakoe
Copy link
Contributor

bakoe commented Feb 1, 2018

bildschirmfoto 2018-02-02 um 00 01 58

@deeps96 Is the design of the “Invite users by e-mail” in the scope of this issue as well?

@bakoe
Copy link
Contributor

bakoe commented Feb 1, 2018

bildschirmfoto 2018-02-02 um 00 06 44

The hover font color of the “Leave team” button has almost no contrast to the background – maybe use something lighter than black? However, white is already “reserved” for the non-hovered button text. Maybe a light blue or something like that for the hover state would be better? cc @zhamanakov

ANSWER: light blue for hover will be ok! DONE

Copy link
Contributor

@bakoe bakoe left a comment

Choose a reason for hiding this comment

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

I just skimmed the code changes since they contain a lot of HTML/CSS stuff for which – to my mind – always many “correct” solutions exist. What I looked into seemed very good. 👍

More intensively, I tested how the re-designed page looks and behaves on Chrome and Safari on macOS. Fortunately, I couldn't find any big bugs and only noticed some minor flaws as documented in the PR's comments section.

@niconomaa
Copy link
Contributor

niconomaa commented Feb 2, 2018

@bakoe to your q whether the email invitiation modal is part of the intended redesign: #381 does not seem to specify that explicitly, even though i'd personally find it bizarre to have this clearly unfinished element on the otherwise so nearly designed page.

Response 1

You are right - it was out of scrope for this story. Maybe it will get an own user story during the Kanban-week? @zhamanakov

ANSWER: It is ok.


@deeps96 even if the invitation part is out of scope for this issue, look at what happens to the top of the page after inviting a user to the team

screen shot 2018-02-02 at 08 50 20

ANSWER: notification re-design is already in progress in #501

@JakobEdding
Copy link
Member

JakobEdding commented Feb 2, 2018

@tomkellog #501 addresses the second issue described by you, it's out of scope for this ticket.

@niconomaa
Copy link
Contributor

niconomaa commented Feb 2, 2018

could there be anything done about the persistent, rectangular highlighting of buttons after clicking on them?

screen shot 2018-02-02 at 08 53 40

Response 1

I think the easiest approach would be, to remove the blue border of this selection completly. @zhamanakov are you ok with that?

ANSWER: yes, just remove the blue border is ok. DONE

content: "*";
vertical-align: 0.5em;
font-size: 0.75em;
font-weight: normal;
Copy link
Contributor

Choose a reason for hiding this comment

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

the css you removed here affected the edit team page, adding an asterisk after required fields, as specified in #218.
please add again!
screen shot 2018-02-02 at 08 58 24

Copy link
Author

Choose a reason for hiding this comment

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

Will fix

Copy link
Author

Choose a reason for hiding this comment

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

Done.

@deeps96
Copy link
Author

deeps96 commented Feb 2, 2018

I wrote some answers into the comments. Waiting for @zhamanakov to reply - will fix everything in one go afterwards.
Also waiting for @bakoe to clarify some things.

@deeps96
Copy link
Author

deeps96 commented Feb 3, 2018

@tomkellog @bakoe @zhamanakov I fixed all things mentioned. Please recheck!

}

.genericButton, .genericButton:hover, .genericButton:focus {
outline: none !important;
Copy link
Contributor

Choose a reason for hiding this comment

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

To my mind, removing the :focus-outline property without adding another visual indication for focused elements is quite an anti-pattern since it reduces the accessibility of the website significantly. See http://www.outlinenone.com for further explanation.

Currently, the “Select all”, “Select none”, “Edit” and “Delete” buttons have a :focus state that looks exactly the same as the non-:focus state. You can inspect this e. g. by Tab-navigating (using the Tab key on you keyboard) over the Team page or by using the Chrome Developer Tools' “Force element state” panel in the Elements tab:

ohne titel

@deeps96 Please make the :focus states of the buttons used on the Team page distinguishable from the normal inactive state. A simple fast solution would be to extend the existing :hover property changes you introduced to changing not only the :hover state, but also the :focus states (e. g. by lightening up the buttons' background colors).

Copy link
Contributor

@bakoe bakoe left a comment

Choose a reason for hiding this comment

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

@deeps96 Thanks for fixing the issues I mentioned! I took another look and can confirm that all the issues are fixed now.

However, I have a minor complaint about the :focus states of some buttons being indistinguishable from the non-focused standard state (see my comment above). Maybe you can talk back to you SM/PO about the importance of accessibility on the page and decide whether you're going to fix this or not.

Response 1

@zhamanakov Whats your opinion?
I would be happy to merge this, as soon as @tomkellog approves that the requested changes from his review has been implemeneted.

@JakobEdding
Copy link
Member

I think @zhamanakov needs to decide on the :focus issue as it's a matter of accessibility and therefore product requirements. Personally, I can understand both the notion of a clean design as well as that of accessibility and supporting different methods of navigating a webpage. Unfortunately, the PO notes on non-functional requirements don't give any input on this either.

@bakoe
Copy link
Contributor

bakoe commented Feb 5, 2018

@jak-ing @deeps96 @zhamanakov

Have you thought about the issue I mentioned in the user story?

#381 (comment)

While reviewing the PR, I noticed that there is no visual indication anymore about whether a team member is a team owner or not. Before the redesign, the user could somehow guess this based on the enabled/disabled state of the “Add ownership” and “Remove ownership” buttons – at least, if the user was the team owner.

Is it intended that there is absolutely no visual indication anymore showing which team member is a team owner?

Response 1

Still waiting for @zhamanakov response.

@deeps96 deeps96 merged commit d79ed0e into dev Feb 5, 2018
@ghost ghost removed the review label Feb 5, 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.

Checkboxes on teams show page are hidden Team Page Design
6 participants