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 #1856 Campaign Alert Table Not Showing under Mobile Layout #1859

Conversation

wendyyuchensun
Copy link
Contributor

@wendyyuchensun wendyyuchensun commented Apr 24, 2018

Fix #1856

The bug

The article column in alert table has a desktop-only-tc class, which hides the whole column under mobile layout.

Since article column should be the only thing alert tables shows under mobile layout, that class makes the table looks like 'totally disappeared' when switches to mobile layout.

What I did

  • Remove the problematic desktop-only-tc class of article column.
  • Also add right border to that column under mobile layout.

Result of fix (viewport width < 920px)

screen shot 2018-04-24 at 9 53 28 pm

Let me know if there is any problem with this PR. Thanks for reviewing!

@wendyyuchensun wendyyuchensun changed the title Fix #1856 Campaign Alert Table Not Shown on Mobile Fix #1856 Campaign Alert Table Not Showing on Mobile Apr 24, 2018
@wendyyuchensun wendyyuchensun changed the title Fix #1856 Campaign Alert Table Not Showing on Mobile Fix #1856 Campaign Alert Table Not Showing under Mobile Layout Apr 24, 2018
@ragesoss
Copy link
Member

@wendyyuchensun cool! I can merge this now if you like, but if you want to take it further, it would be ideal to include more info in the mobile layout. The most useful thing would be to at least show the alert type.

@wendyyuchensun wendyyuchensun force-pushed the fix-campaign-alert-table-on-mobile branch from 39d8826 to 5dbcde7 Compare April 25, 2018 01:50
@wendyyuchensun wendyyuchensun force-pushed the fix-campaign-alert-table-on-mobile branch 2 times, most recently from 5ee486a to 3b80c59 Compare April 25, 2018 01:58
@wendyyuchensun wendyyuchensun force-pushed the fix-campaign-alert-table-on-mobile branch from 3b80c59 to a7cb23f Compare April 25, 2018 02:49
@wendyyuchensun
Copy link
Contributor Author

wendyyuchensun commented Apr 25, 2018

Hi @ragesoss, I've added a new commit to include type column in mobile layout.

screen shot 2018-04-25 at 9 31 33 am

However, CI failed. I think it's related to version of Node used in CI (v10.0.0), not this PR. I reproduced same error shown in CI logs by using Node 10.0.0 and running gulp related commands in my local environment (macOS/gulp 3.9.1). Using Node 8.11.1 works fine.

Seems like someone bumped into similar problem: gulpjs/gulp#2162

@ragesoss
Copy link
Member

@wendyyuchensun thanks! I'll look into the node/gulp issue soon, probably tomorrow. Did you try upgrading gulp to 4.0.0?

@wendyyuchensun
Copy link
Contributor Author

Not yet. I'll see if I have time to try it out recently. I'll share the result here when I'm done.

@ragesoss
Copy link
Member

The nodejs 10 compatibility is sorted out now... lemme give this a try.

@ragesoss ragesoss merged commit 50dfddb into WikiEducationFoundation:master Apr 26, 2018
@ragesoss
Copy link
Member

Nice work @wendyyuchensun. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Campaign Alerts list is not shown on mobile
2 participants