Skip to content

Add mobile friendly table of content #564

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

Merged
merged 3 commits into from
Jun 19, 2020

Conversation

rvsia
Copy link
Contributor

@rvsia rvsia commented Jun 18, 2020

On mobile phone the content is put to the end of pages and it's not very useful (especially on long screens):

image

This PR hides this list on smaller screen and puts a menu to the top of the page:

showcontent

  • fix a bug when the code editor was bigger than the viewport.

@rvsia rvsia added enhancement New feature or request docs docs pull requests labels Jun 18, 2020
@rvsia rvsia requested a review from Hyperkid123 June 18, 2020 11:02
@codecov
Copy link

codecov bot commented Jun 18, 2020

Codecov Report

Merging #564 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #564   +/-   ##
=======================================
  Coverage   93.73%   93.73%           
=======================================
  Files         189      189           
  Lines        2906     2906           
  Branches      952      952           
=======================================
  Hits         2724     2724           
  Misses        182      182           

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 b90d156...40516fd. Read the comment docs.

@Hyperkid123
Copy link
Member

@rvsia nice
I think we need to emphasize the buttons bit. I think its easy to miss

@rvsia
Copy link
Contributor Author

rvsia commented Jun 18, 2020

@Hyperkid123 Yeah, I am thinking the same. However, do you have any idea? icon, make it bold? It should be visible, but not so much. :D

@Hyperkid123
Copy link
Member

Hyperkid123 commented Jun 18, 2020

Well, the next question is. Do we even need that in the mobile view? Should we just hide the whole thing?

Usually, this kind of navigation is stacked on top before the content or just hidden. I think it adds too much complexity. In our case, sometimes we have a lot of sections and the content can get pretty long. I would just hide it on mobile...

@rvsia
Copy link
Contributor Author

rvsia commented Jun 18, 2020

I think it's a nice feature to have - especially on pages with a lot sections. Users can quickly scan the whole content to find if the page contains what they are searching for. And having it as a small button does not disrupt the look and the feel of the page.

@Hyperkid123
Copy link
Member

All right let's try to find something that also looks good.

@rvsia
Copy link
Contributor Author

rvsia commented Jun 18, 2020

icons

What about? (not pushed yet)

@rvsia
Copy link
Contributor Author

rvsia commented Jun 18, 2020

Maybe with pushing it to the right side

@Hyperkid123
Copy link
Member

I agree it should be on the right side

@rvsia
Copy link
Contributor Author

rvsia commented Jun 18, 2020

icons


<Grid container item>

<ListOfContentsMobile file="mappers/blueprint-component-mapper" />
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 exactly like that we need a special component.
We can use smart layout and reverse the column/row order on small screens. And make this a part of the normal content component.

It should work since we have a different grid item for the actual content and the content navigation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would like to make a common layout component in the next PR and refactor all pages to use it. I could include this change in it.

import DocPage from '@docs/doc-page'

<DocPage file="bla/bla">
  ...content...
</DocPage>

instead of defining Grids and contents in every page.

Copy link
Member

Choose a reason for hiding this comment

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

Yep, that is exactly what should happen. Can you add a task to the docs project?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done! 🖖 you can find the task in the issues. I will start working on it right after this is merged.

@Hyperkid123 Hyperkid123 merged commit c37d044 into data-driven-forms:master Jun 19, 2020
@Hyperkid123
Copy link
Member

🎉 This PR is included in version 2.5.2 🎉

The release is available on

Demo can be found here!

@rvsia rvsia deleted the addMobileContent branch July 10, 2020 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs docs pull requests enhancement New feature or request released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants