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

Adds LUX header and footer #414

Closed
wants to merge 18 commits into from
Closed

Adds LUX header and footer #414

wants to merge 18 commits into from

Conversation

axamei
Copy link
Contributor

@axamei axamei commented Jul 26, 2018

No description provided.

@tpendragon
Copy link
Contributor

We'll need to install yarn on dpul-staging1 to deploy this.

@tpendragon
Copy link
Contributor

tpendragon commented Jul 26, 2018

Tried to use yarn for deploy and got an error: error lux-design-system@0.0.14: The engine "node" is incompatible with this module. Expected version ">= 9.11.1"..

NPM works, so sticking with that. Maybe something you can look at @axamei if we want to stick with YARN?

Update: Looking into this now, I think it's just a yarn installation problem.

@coveralls
Copy link

coveralls commented Jul 26, 2018

Coverage Status

Coverage decreased (-0.3%) to 99.693% when pulling f88295b on with_lux into 9242a7c on master.

@tpendragon
Copy link
Contributor

Dropdown user menu is being hidden:
screen shot 2018-07-26 at 10 18 54 am

@tpendragon
Copy link
Contributor

tpendragon commented Jul 26, 2018

Show views are broken:

screen shot 2018-07-26 at 10 20 36 am

(This might not be this PR's fault - testing master now)

Confirmed the view is fine in master.

@tpendragon
Copy link
Contributor

pulibrary/princeton_ansible#300 is necessary to deploy this.

@escowles
Copy link
Member

I deployed this to staging and tested it compared to the master branch. I saw some of the same display issues that @tpendragon mentioned above:

  • While the page was loading, the header is missing
  • On the item show page, the bookmark/email/sms links are unstyled
  • If the item show page is loaded using turbolinks, the viewer doesn't display. If I reload the page or enter the URL in my browser, the viewer works fine.

I did some basic timings and saved the pages with dependencies using Chrome, and found the Lux versions were a little slower and larger. But turbolinks is a significant factor there, so we should probably talk about the best methodology for testing page sizes and load times.

@sdellis
Copy link
Member

sdellis commented Jul 30, 2018

@escowles this was branched from master before the header/footer rollback. Those issues may be related to problems with that branch. We are going to redo this from master. @axamei is out today. Can we keep this open until she returns?

Re: the benchmarking... Lux is going to add some additional weight while we transition since we are only adding, not yet taking anything away. I agree that we should discuss the best methodology for testing. I think Lighthouse audits are a pretty good way to do this.

@escowles
Copy link
Member

@sdellis It's totally fine to leave this open a while (as long as you think it's worth having around in fact). Basing off the updated master branch makes sense, and hopefully resolves some of the styling issues.

I'll take a look at Lighthouse. I don't think modest page size or load time increases would be a problem (in fact, that's what I expect because we're adding Lux and not removing anything else yet). I was mostly just curious about what the impact was.

@axamei
Copy link
Contributor Author

axamei commented Aug 3, 2018

Going to start a new branch from the current master. This is too messy.

@axamei axamei mentioned this pull request Aug 3, 2018
@axamei
Copy link
Contributor Author

axamei commented Aug 3, 2018

Feel free to close in favor of #418

@tpendragon
Copy link
Contributor

Closed in favor of #418

@tpendragon tpendragon closed this Aug 3, 2018
@tpendragon tpendragon removed the review label Aug 3, 2018
@hackartisan hackartisan deleted the with_lux branch July 22, 2021 18:12
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.

6 participants