Skip to content

Update Dash logos in all dashBio apps#265

Merged
rpkyle merged 20 commits intomasterfrom
change-dashbio-logo
Aug 8, 2019
Merged

Update Dash logos in all dashBio apps#265
rpkyle merged 20 commits intomasterfrom
change-dashbio-logo

Conversation

@HammadTheOne
Copy link
Copy Markdown
Contributor

Issue - Dash Bio app logo's have been updated to reflect the new Plotly - Dash logo in the python version. This PR fixes the headers and updates the logo for all Dash-Bio R app versions and adds the logo to their corresponding assets.

App pull request

  • This is a new app
  • I am improving an existing app (redesigns/code "makeovers")

Workflow

  • I have created a branch in the appropriate monorepo, and the
    elements necessary for successful deployment are in place.
  • If the app is a redesigned and/or restyled version of an
    existing gallery app, I've summarized the changes requested in the
    appropriate Streambed issue and confirm that they have been applied.
  • If the app is on the Dash Gallery portal, I have added a link to
    the GitHub repository for the source code in the portal description.
  • If the app is a reimplementation of a Python gallery app for the
    DashR gallery, the app in this PR mimics, as closely as possible,
    the style and functionality of the existing app.=
  • I have removed all Google Analytics code from the app's
    assets/ folder.

The pre-review review

I have addressed all of the following questions:

  • Does everything in my code serve some purpose? (I have removed
    any dead and/or irrelevant code.)
  • Does everything in my code have a clear purpose? (My code is
    readable and, where it isn't, it has been commented appropriately.)]
  • Am I reinventing the wheel? (I have used appropriate packages to
    lessen the volume of code that needs to be maintained.)

Comment thread apps/dashr-clustergram/app.R Outdated
Comment thread apps/dashr-financial-report/app.R Outdated


dividends_table <- generate_table(remove.factors(dividends_data))
dividends_table <- generate_table(dividends_data)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not sure why this diff is here -- this PR looks like it's for the Dash Bio apps. Might be a merge issue with master?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I was just going through all apps I'd worked on to check the logo used on them and noticed this small bug. There's no real changes with the app itself, but should I open a separate PR for this?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@HammadTheOne Is it possible to ignore this file and remove the directory from your commit (without erasing the existing app, of course)? Even if it triggers a conflict, we can resolve it before merging.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Should be fixed now, I reverted the change from this commit. Is that OK?

@rpkyle rpkyle changed the title Change dashbio logo Update Dash logos in all dashBio apps Aug 7, 2019
@rpkyle rpkyle added the dash-r Dash R applications label Aug 7, 2019
Copy link
Copy Markdown
Contributor

@rpkyle rpkyle left a comment

Choose a reason for hiding this comment

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

Thanks for fixing up the CSS font issue, and adding the logo to dashr-alignment-viewer.

I'm seeing some CSS positioning issues with Needle Plot and Speck, but those are out of scope for your PR. I think we're ready to 💃 assuming the font 404 errors are gone.

width = '50',
style = list('top' = '10', 'margin-left' = '10px')),
children = list(
htmlImg(src='assets/plotly-dash-bio-logo.png', height = '36', width = '180',
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@HammadTheOne Can we confirm that this file exists in assets? I don't think I see it in my local clone.

Copy link
Copy Markdown
Contributor

@shammamah-zz shammamah-zz left a comment

Choose a reason for hiding this comment

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

💃 It looks good to me!

@rpkyle rpkyle merged commit 94d08db into master Aug 8, 2019
@rpkyle rpkyle deleted the change-dashbio-logo branch August 8, 2019 19:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dash-r Dash R applications

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants