Skip to content
This repository has been archived by the owner on May 19, 2020. It is now read-only.

feat(navigation): restyle navigation sizing - I276 #313

Conversation

jerrybuks
Copy link
Contributor

Signed-off-by: jerrybuks [email protected]

Issue #

Made the width of the navigation component adjust based on screen size and used CSS to determine when to truncate & add the ellipsis.### Changes

  • Updated generators.js
  • Updated Navigation/styles.js

Flags

-Headings in navigation can appear to be truncated prematurely, especially on larger screens

Related Issues

  • N/A

@elit-altum
Copy link
Contributor

@jerrybuks Could you add some screenshots/GIFs of the changes made?

@jerrybuks
Copy link
Contributor Author

@jerrybuks Could you add some screenshots/GIFs of the changes made?

But that can be found, under files changed

@jolanglinais
Copy link
Member

Looks promising at first glance, will test this more on Monday.

@Michael-Grover
Copy link
Collaborator

@jerrybuks can you provide a gif or video of the truncation in practice? Possible showing different screen widths and how it affects the navigation sizing? I recommend using https://www.loom.com or Quicktime to record

@jerrybuks
Copy link
Contributor Author

@jerrybuks can you provide a gif or video of the truncation in practice? Possible showing different screen widths and how it affects the navigation sizing? I recommend using https://www.loom.com or Quicktime to record

okay

@jolanglinais
Copy link
Member

It looks like it is not truncating but rather just cutting it off at a point:

Screen Shot 2020-03-02 at 10 35 49 AM

@jerrybuks
Copy link
Contributor Author

It looks like it is not truncating but rather just cutting it off at a point:

Screen Shot 2020-03-02 at 10 35 49 AM

It truncates, but I just noticed this issue happens when the view port width is greater than the width of the left Nav, since the left Nav is set to "overflow:hidden", it displays this way. Noticed It about 30 minutes ago, I'm about to update this PR to fix this issue.

@jerrybuks
Copy link
Contributor Author

It looks like it is not truncating but rather just cutting it off at a point:
Screen Shot 2020-03-02 at 10 35 49 AM

It truncates, but I just noticed this issue happens when the view port width is greater than the width of the left Nav, since the left Nav is set to "overflow:hidden", it displays this way. Noticed It about 30 minutes ago, I'm about to update this PR to fix this issue.

fixed!

@jerrybuks
Copy link
Contributor Author

It looks like it is not truncating but rather just cutting it off at a point:
Screen Shot 2020-03-02 at 10 35 49 AM

It truncates, but I just noticed this issue happens when the view port width is greater than the width of the left Nav, since the left Nav is set to "overflow:hidden", it displays this way. Noticed It about 30 minutes ago, I'm about to update this PR to fix this issue.

fixed!

To make the width of the Navigation bigger, so as to completely prevent truncating when there is still enough space on the page, this will have to be done in the leftNav component in the "template-studio-v2", trying to do this from the Navigation component causes the issue you noticed, as the Left Nav component has its overflow set to hidden, so If the width of the Navigation component becomes bigger than its own width, it hides the content of the Navigation. with the changes I just made, the Navigation now inherits the width of its parent.

@jolanglinais
Copy link
Member

Latest:

Screen Shot 2020-03-02 at 11 16 20 AM

@jerrybuks
Copy link
Contributor Author

jerrybuks commented Mar 2, 2020

@jerrybuks can you provide a gif or video of the truncation in practice? Possible showing different screen widths and how it affects the navigation sizing? I recommend using https://www.loom.com or Quicktime to record

okay
Here, https://www.loom.com/share/24fe32d37c724fecbcfdc10bbfabdca8

@jolanglinais
Copy link
Member

I think this means you can remove generators.js and fix/remove the tests that relied on this logic as well.

@jerrybuks
Copy link
Contributor Author

I think this means you can remove generators.js and fix/remove the tests that relied on this logic as well.

okay, will do that

@jerrybuks
Copy link
Contributor Author

I think this means you can remove generators.js and fix/remove the tests that relied on this logic as well.

Done, supposed to be action.js(deleted), though and I've also removed the tests for It

DianaLease
DianaLease previously approved these changes Mar 2, 2020
Copy link
Member

@DianaLease DianaLease left a comment

Choose a reason for hiding this comment

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

LGTM

jolanglinais
jolanglinais previously approved these changes Mar 2, 2020
Copy link
Member

@jolanglinais jolanglinais left a comment

Choose a reason for hiding this comment

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

⚡️

@jolanglinais
Copy link
Member

@jerrybuks could you just rebase with the latest master and I'll merge?

@jerrybuks jerrybuks dismissed stale reviews from jolanglinais and DianaLease via 73c7baa March 3, 2020 03:44
@jerrybuks jerrybuks force-pushed the jerrybuks-issue276-restyling-navigation-sizing branch from 36eb721 to 73c7baa Compare March 3, 2020 03:44
@jerrybuks
Copy link
Contributor Author

jerrybuks commented Mar 3, 2020

@jerrybuks could you just rebase with the latest master and I'll merge?

Okay, I think I've done that, by running " git rebase upstream/master" and then pushing, I this what you meant? or please do you mean pulling and merging from upstream/master

@jolanglinais
Copy link
Member

Looks like you're in a fork. Go with:

  git checkout master
  git fetch --all --prune
  git rebase upstream/master
  git push origin master
  git checkout jerrybuks-issue276-restyling-navigation-sizing
  git rebase master
  git push origin jerrybuks-issue276-restyling-navigation-sizing -f

@jerrybuks jerrybuks force-pushed the jerrybuks-issue276-restyling-navigation-sizing branch from 73c7baa to 86ccd03 Compare March 4, 2020 11:58
@jerrybuks
Copy link
Contributor Author

Looks like you're in a fork. Go with:

  git checkout master
  git fetch --all --prune
  git rebase upstream/master
  git push origin master
  git checkout jerrybuks-issue276-restyling-navigation-sizing
  git rebase master
  git push origin jerrybuks-issue276-restyling-navigation-sizing -f

Done @irmerk

@jolanglinais jolanglinais merged commit fb1925c into accordproject:master Mar 4, 2020
@jerrybuks jerrybuks deleted the jerrybuks-issue276-restyling-navigation-sizing branch March 9, 2020 21:10
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Restyle navigation sizing
5 participants