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

fix: making side menu sticky #3621

Merged
merged 9 commits into from
Feb 13, 2020
Merged

fix: making side menu sticky #3621

merged 9 commits into from
Feb 13, 2020

Conversation

maze-runnar
Copy link
Contributor

@maze-runnar maze-runnar commented Nov 11, 2019

Fixes #3510
Making side bar sticky .
working fine on computers :
ezgif com-gif-maker (3)
And Mobile Device :
Screenshot from 2019-11-11 15-23-09

Checklist

  • I have read the Contribution & Best practices Guide.
  • My branch is up-to-date with the Upstream development branch.
  • The acceptance, integration, unit tests and linter pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

@auto-label auto-label bot added the fix label Nov 11, 2019
@maze-runnar
Copy link
Contributor Author

@kushthedude @uds5501 please review this .

uds5501
uds5501 previously approved these changes Nov 12, 2019
Copy link
Contributor

@uds5501 uds5501 left a comment

Choose a reason for hiding this comment

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

Works fine.

@kushthedude
Copy link
Member

I think you didn't get the issue, We have to make it stick to the top irrespective of scrolls. Using rails the navbar is stick to the top and to navigate to other option I had to scroll up back.

@kushthedude
Copy link
Member

See the second image on issue description, We have to make sure that doesn't happen.

@maze-runnar
Copy link
Contributor Author

maze-runnar commented Nov 13, 2019

See the second image on issue description, We have to make sure that doesn't happen.

@kushthedude i have seen the issue description's second image . but once i refresh the page it disappeared .Making it sticky will be solve that problem (as user is not going to refresh the page every time:smile:) . As in profile section also the side menu is sticky and one have to scroll back to change the page(at https://eventyay.com/account/billing/payment-info). I think there should be no problem in scrolling back as they do in mobile phones .
Screenshot from 2019-11-13 19-55-40

@kushthedude
Copy link
Member

See the second image on issue description, We have to make sure that doesn't happen.

@kushthedude i have seen the issue description's second image . but once i refresh the page it disappeared .Making it sticky will be solve that problem (as user is not going to refresh the page every time😄) . As in profile section also the side menu is sticky and one have to scroll back to change the page(at https://eventyay.com/account/billing/payment-info). I think there should be no problem in scrolling back as they do in mobile phones .
Screenshot from 2019-11-13 19-55-40

The sidebar should be stick to the top, irrespective of user scrolls or not.

@maze-runnar
Copy link
Contributor Author

maze-runnar commented Dec 5, 2019

@kushthedude please take a look at this .may be This will make clear what i want to say about ui-sticky .
https://stackoverflow.com/questions/51102492/refreshing-semantic-ui-sticky-with-ember-does-not-work

@iamareebjamal
Copy link
Member

@maze-runnar Use position sticky

@maze-runnar
Copy link
Contributor Author

Ok

@maze-runnar
Copy link
Contributor Author

maze-runnar commented Feb 13, 2020

@maze-runnar Use position sticky

@iamareebjamal position: sticky is also not working. cause ui menu uses flex-box (https://github.com/Semantic-Org/Semantic-UI/blob/master/dist/components/menu.min.css) and sticky not works in this case.https://stackoverflow.com/questions/43707076/position-sticky-not-working-css-and-html/47878455

What i should do next?

@iamareebjamal
Copy link
Member

See if there is overflow hidden in parent

@maze-runnar
Copy link
Contributor Author

See if there is overflow hidden in parent

ok

@maze-runnar
Copy link
Contributor Author

@iamareebjamal actual problem is here :

{{#ui-sticky context='#public-event-content' observeChanges=true}}
       {{public/side-menu event=model}}
 {{/ui-sticky}}

in {{ui-sticky}}.

I used

div class="ui fluid" style="position: sticky; top:0px;">
      {{public/side-menu event=model}}
</div>

and it resolves the issue. should i remove {{ui-sticky}} ?

@iamareebjamal
Copy link
Member

Yes, wherever used

@codecov
Copy link

codecov bot commented Feb 13, 2020

Codecov Report

❗ No coverage uploaded for pull request base (development@b339938). Click here to learn what that means.
The diff coverage is 13.33%.

Impacted file tree graph

@@              Coverage Diff               @@
##             development    #3621   +/-   ##
==============================================
  Coverage               ?   21.89%           
==============================================
  Files                  ?      460           
  Lines                  ?     4736           
  Branches               ?        0           
==============================================
  Hits                   ?     1037           
  Misses                 ?     3699           
  Partials               ?        0
Impacted Files Coverage Δ
...ntrollers/events/view/tickets/access-codes/list.js 0% <ø> (ø)
...omponents/forms/admin/content/social-links-form.js 0% <ø> (ø)
app/mixins/custom-form.js 0% <ø> (ø)
app/routes/events/view/speakers/list.js 0% <ø> (ø)
app/routes/account/billing/payment-info.js 100% <ø> (ø)
app/routes/events/list.js 90.47% <ø> (ø)
app/models/event.js 53.84% <ø> (ø)
app/components/tables/utilities/pagination.js 57.14% <ø> (ø)
app/components/forms/admin/settings/system-form.js 0% <ø> (ø)
app/routes/events/view/sessions/create.js 0% <ø> (ø)
... and 133 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b339938...2380ce2. Read the comment docs.

@@ -35,9 +35,9 @@
{{/if}}
{{/if}}
{{else}}
{{#ui-sticky context='#public-event-content' observeChanges=true}}
<div class="ui fluid" style="position:sticky; top:0px;">
Copy link
Member

Choose a reason for hiding this comment

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

I don't think top 0px is right. Besides, it should be moved to CSS file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's working on top:0px - https://youtu.be/8cqPH3b4X58
and if i should change then what should be the value.

iamareebjamal
iamareebjamal previously approved these changes Feb 13, 2020
@iamareebjamal
Copy link
Member

@maze-runnar top: 14px will look better

@iamareebjamal iamareebjamal merged commit 67c0bdd into fossasia:development Feb 13, 2020
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.

The sidebar is not sticky and the images on speakers page are all of different sizes.
4 participants