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

Update links in README.md to point to Element #14973

Merged
merged 4 commits into from
Aug 24, 2020

Conversation

Weasy666
Copy link
Contributor

There still one or two links pointing to Riot specific stuff, as there exists no equivalent for Element, at least as far as i know.

These are:

@t3chguy t3chguy requested a review from a team August 17, 2020 09:43
Copy link
Collaborator

@jryans jryans 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 helping with this! 😄 A few comments below.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@jryans jryans left a comment

Choose a reason for hiding this comment

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

There's a few more references to the Docker image that need to revert for now.

README.md Outdated
would be:
```bash
docker run -p 80:80 -v /etc/riot-web/config.json:/app/config.json vectorim/riot-web
docker run -p 80:80 -v /etc/element-web/config.json:/app/config.json vectorim/element-web
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
docker run -p 80:80 -v /etc/element-web/config.json:/app/config.json vectorim/element-web
docker run -p 80:80 -v /etc/element-web/config.json:/app/config.json vectorim/riot-web

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh man....i'm really sorry for being so blind 🙈😓
will get to it ASAP

README.md Outdated
git checkout master
docker build -t vectorim/riot-web .
docker build -t vectorim/element-web .
Copy link
Collaborator

@jryans jryans Aug 19, 2020

Choose a reason for hiding this comment

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

Suggested change
docker build -t vectorim/element-web .
docker build .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm...this is a docker build command, so couldn't we leave it at element-web? Or do you think user could be confused about the alternating use of element-web and riot-web?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah good point! Actually, I kind of think it would make sense to just omit the tag entirely...? If you are building the image yourself, you probably want to tag it differently anyway. So, I'd say remove the -t arg here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

(Updated suggestion to match.)

README.md Outdated
```bash
docker build -t vectorim/riot-web:develop \
docker build -t vectorim/element-web:develop \
Copy link
Collaborator

@jryans jryans Aug 19, 2020

Choose a reason for hiding this comment

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

Suggested change
docker build -t vectorim/element-web:develop \
docker build \

Copy link
Collaborator

@jryans jryans 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 all the revisions, looks great to me. 😄

@jryans jryans merged commit b99b647 into element-hq:develop Aug 24, 2020
@Weasy666
Copy link
Contributor Author

Thanks for the patience 😅
And you are welcome.

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.

2 participants