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

Remove the mobile hidden class #5050

Merged
merged 1 commit into from Oct 24, 2018

Conversation

Projects
None yet
5 participants
@SychO9
Contributor

SychO9 commented Oct 12, 2018

Signed-off-by: SychO sychocouldy@gmail.com

Remove the mobile hidden class
Signed-off-by: SychO <sychocouldy@gmail.com>
@Sesquipedalian

This comment has been minimized.

Member

Sesquipedalian commented Oct 12, 2018

Is there a particular issue that this is meant to address?

@SychO9

This comment has been minimized.

Contributor

SychO9 commented Oct 12, 2018

Basically the hidden class was introduced to replace "style='display:none;'" especially for js #5036
I found out that the hidden class was also used to hide elements on mobile view, which is something we don't want to have. so this pr removes that specific usage and hides the elements in a better way.

Incase you meant if there were existing issues addressing this, there aren't.

@jdarwood007

This comment has been minimized.

Member

jdarwood007 commented Oct 13, 2018

@Gwenwyfar Thoughts?

Sounds like a fix then. We should have checked that, but hidden was too generic of a class for targeting just mobile usage. Especially considering its global usage across other projects.

@SychO9

This comment has been minimized.

Contributor

SychO9 commented Oct 13, 2018

Indeed, good thing it was only used in these couple of files, makes it easier to get rid of.

@Antes

This comment has been minimized.

Contributor

Antes commented Oct 13, 2018

or you could have renamed hidden to mobile_hidden :) using less generic classes in core template is not really a good way to do things.

@SychO9

This comment has been minimized.

Contributor

SychO9 commented Oct 13, 2018

True, but considering the CSS currently hides elements using unique class names and not generic ones, it'd be somewhat messy to add a generic class and use it in only a couple of files.

However it'd be very practical for mods to have a few classes that hide elements in different screen sizes.

@Antes

This comment has been minimized.

Contributor

Antes commented Oct 14, 2018

For the record, first draft of responsive(css) was very hardcoded, end shape was designed to use more generic classes, but since I had less time and motivation it stayed as is. There should be mobile_480 / mobile_720 / mobile_hidden to hide things not #ID > Class

@SychO9

This comment has been minimized.

Contributor

SychO9 commented Oct 14, 2018

I also use generic classes as such for my themes.
Whether we should use generic classes or not is something that needs discussion, I'm only following how the team does things
#4953 (review)

And if it was decided that generic classes Should be used. I'd make the changes.
There are advantages in both using them and not in my opinion so I don't mind either ways.

@Gwenwyfar Gwenwyfar added the Theme label Oct 24, 2018

@Gwenwyfar

This comment has been minimized.

Collaborator

Gwenwyfar commented Oct 24, 2018

Using mobile_hidden classes isn't very semantic nor very practical/versatile, so this looks good to me. We could certainly add them in the css for use by mods and custom themes but I don't think we should use them ourselves.

@jdarwood007 jdarwood007 merged commit 1413be1 into SimpleMachines:release-2.1 Oct 24, 2018

2 checks passed

Scrutinizer No new issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@SychO9 SychO9 deleted the SychO9:mobileHidden branch Oct 26, 2018

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