Skip to content

Conversation

@drewbutlerbb4
Copy link
Contributor

Signed-off-by: Andrew-Butler Andrew.Butler@ibm.com

Related: #168

(prio 1) Join the conversation / Notebooks links are overlapping in Firefox 90.0.2 (64-bit) - RHEL 8.3
please just make it part of the list of asset types

Addresses issue mentioned in #168 as well as making the settings button more noticeable and some extra css changes to address other sidebar issues.

For large screens:
Screen Shot 2021-08-18 at 11 44 04 AM
For small screens:
Screen Shot 2021-08-18 at 11 44 30 AM

Signed-off-by: Andrew-Butler <Andrew.Butler@ibm.com>
Copy link
Contributor

@ckadner ckadner left a comment

Choose a reason for hiding this comment

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

Thanks Andrew. This looks good to me. I found just a missing line break in one file.

The "Join the Conversation" button seems to break the formatting a bit. Should we maybe break the text into two lines and align it with the menu items on the left side? Same with "Settings" I suppose.

Did you want @romeokienzler or @animeshsingh to review this before merging?

Signed-off-by: Andrew-Butler <Andrew.Butler@ibm.com>
@drewbutlerbb4
Copy link
Contributor Author

@ckadner

The "Join the Conversation" button seems to break the formatting a bit. Should we maybe break the text into two lines and align it with the menu items on the left side? Same with "Settings" I suppose.

Here is what it looks like with your recommended formatting.
Screen Shot 2021-08-30 at 8 24 58 AM

@ckadner
Copy link
Contributor

ckadner commented Aug 31, 2021

@ckadner

The "Join the Conversation" button seems to break the formatting a bit. Should we maybe break the text into two lines and align it with the menu items on the left side? Same with "Settings" I suppose.

I like that better. Maybe the "Conversation" word should align with "join" instead of aligning with the speech bubble icon

@ckadner ckadner added the UI User Interface label Aug 31, 2021
@ckadner
Copy link
Contributor

ckadner commented Aug 31, 2021

What happens if the screen goes even smaller? Does the Setting and Join the Conversation overlap the asset category links? Instead the whole menu should scroll maybe?

Signed-off-by: Andrew-Butler <Andrew.Butler@ibm.com>
@drewbutlerbb4
Copy link
Contributor Author

@ckadner this is the updated look. It is not perfect, but it removes some of the issues. Will follow up with a PR for a scrollable sidebar when I get extra time.
Non-admin view:
Screen Shot 2021-09-01 at 11 19 32 AM
Admin view:
Screen Shot 2021-09-01 at 11 20 45 AM
Admin view with open secret menu:
Screen Shot 2021-09-01 at 11 20 53 AM

@ckadner
Copy link
Contributor

ckadner commented Sep 1, 2021

Cool. Thank you Andrew. This looks good. We could make two small changes in a follow up, like:

  • let the "Join the Conversation" link sit at the bottom for non-admins, and push it up with the padding you have here for logged-in admins
  • align the left side of the word "Conversation" with "Join" in the line above

@ckadner ckadner mentioned this pull request Sep 1, 2021
2 tasks
Copy link
Contributor

@ckadner ckadner left a comment

Choose a reason for hiding this comment

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

/lgtm

@mlx-bot
Copy link
Collaborator

mlx-bot commented Sep 1, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ckadner, drewbutlerbb4

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:
  • OWNERS [ckadner,drewbutlerbb4]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ckadner ckadner merged commit 037180b into claimed-framework:main Sep 1, 2021
@romeokienzler
Copy link
Member

@drewbutlerbb4 @ckadner this is still not 100% working out

simplescreenrecorder-2021-09-17_09.32.40.mp4

@romeokienzler
Copy link
Member

simplescreenrecorder-2021-09-17_15.43.31.mp4

@romeokienzler
Copy link
Member

thanks @drewbutlerbb4 & @ckadner - tested - works like a charm!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants